[Qemu-block] [PATCH] nbd: Grab aio context lock in more places

[Eric Blake]

When iothreads are in use, the failure to grab the aio context results
in an assertion failure when trying to unlock things during blk_unref,
when trying to unlock a mutex that was not locked.  In short, all
calls to nbd_export_put need to done while within the correct aio
context.  But since nbd_export_put can recursively reach itself via
nbd_export_close, and recursively grabbing the context would deadlock,
we can't do the context grab directly in those functions, but must do
so in their callers.

Hoist the use of the correct aio_context from nbd_export_new() to its
caller qmp_nbd_server_add().  Then tweak qmp_nbd_server_remove(),
nbd_eject_notifier(), and nbd_esport_close_all() to grab the right
context, so that all callers during qemu now own the context before
nbd_export_put() can call blk_unref().

Remaining uses in qemu-nbd don't matter (since that use case does not
support iothreads).

Suggested-by: Kevin Wolf 
Signed-off-by: Eric Blake 
---

With this in place, my emailed formula [1] for causing an iothread
assertion failure no longer hits, and all the -nbd and -qcow2 iotests
still pass.  I would still like to update iotests to cover things (I
could not quickly figure out how to make iotest 222 use iothreads -
either we modify that one or add a new one), but wanted to get review
started on this first.

[1] https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03383.html

 include/block/nbd.h |  1 +
 blockdev-nbd.c  | 14 --
 nbd/server.c| 23 +++
 3 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 21550747cf35..316fd705a9e4 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -340,6 +340,7 @@ void nbd_export_put(NBDExport *exp);

 BlockBackend *nbd_export_get_blockdev(NBDExport *exp);

+AioContext *nbd_export_aio_context(NBDExport *exp);
 NBDExport *nbd_export_find(const char *name);
 void nbd_export_close_all(void);

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 213f226ac1c4..6a8b206e1d74 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -151,6 +151,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, 
const char *name,
 BlockBackend *on_eject_blk;
 NBDExport *exp;
 int64_t len;
+AioContext *aio_context;

 if (!nbd_server) {
 error_setg(errp, "NBD server not running");
@@ -173,11 +174,13 @@ void qmp_nbd_server_add(const char *device, bool 
has_name, const char *name,
 return;
 }

+aio_context = bdrv_get_aio_context(bs);
+aio_context_acquire(aio_context);
 len = bdrv_getlength(bs);
 if (len < 0) {
 error_setg_errno(errp, -len,
  "Failed to determine the NBD export's length");
-return;
+goto out;
 }

 if (!has_writable) {
@@ -190,13 +193,16 @@ void qmp_nbd_server_add(const char *device, bool 
has_name, const char *name,
 exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, !writable, !writable,
  NULL, false, on_eject_blk, errp);
 if (!exp) {
-return;
+goto out;
 }

 /* The list of named exports has a strong reference to this export now and
  * our only way of accessing it is through nbd_export_find(), so we can 
drop
  * the strong reference that is @exp. */
 nbd_export_put(exp);
+
+ out:
+aio_context_release(aio_context);
 }

 void qmp_nbd_server_remove(const char *name,
@@ -204,6 +210,7 @@ void qmp_nbd_server_remove(const char *name,
Error **errp)
 {
 NBDExport *exp;
+AioContext *aio_context;

 if (!nbd_server) {
 error_setg(errp, "NBD server not running");
@@ -220,7 +227,10 @@ void qmp_nbd_server_remove(const char *name,
 mode = NBD_SERVER_REMOVE_MODE_SAFE;
 }

+aio_context = nbd_export_aio_context(exp);
+aio_context_acquire(aio_context);
 nbd_export_remove(exp, mode, errp);
+aio_context_release(aio_context);
 }

 void qmp_nbd_server_stop(Error **errp)
diff --git a/nbd/server.c b/nbd/server.c
index 378784c1e54a..3003381c86b4 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1458,7 +1458,12 @@ static void blk_aio_detach(void *opaque)
 static void nbd_eject_notifier(Notifier *n, void *data)
 {
 NBDExport *exp = container_of(n, NBDExport, eject_notifier);
+AioContext *aio_context;
+
+aio_context = exp->ctx;
+aio_context_acquire(aio_context);
 nbd_export_close(exp);
+aio_context_release(aio_context);
 }

 NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
@@ -1477,12 +1482,11 @@ NBDExport *nbd_export_new(BlockDriverState *bs, 
uint64_t dev_offset,
  * NBD exports are used for non-shared storage migration.  Make sure
  * that BDRV_O_INACTIVE is cleared and the image is ready for write
  * access since the export could be available before migration handover.
+ * ctx was acquired in the caller.
  */
 assert(name);
 ctx = bdrv_get_aio_context(bs);
-

Re: [Qemu-block] [PATCH] nbd/server: attach client channel to the export's AioContext

[Eric Blake]

On 9/12/19 6:31 AM, Kevin Wolf wrote:

>>
>> Yes, I think locking the context during the "if (exp->blk) {" block at
>> nbd/server.c:1646 should do the trick.

That line number has moved over time; which function are you referring to?

> 
> We need to be careful to avoid locking things twice, so maybe
> nbd_export_put() is already too deep inside the NBD server.
> 
> Its callers are:
> 
> * qmp_nbd_server_add(). Like all other QMP handlers in blockdev-nbd.c it
>   neglects to lock the AioContext, but it should do so. The lock is not
>   only needed for the nbd_export_put() call, but even before.
> 
> * nbd_export_close(), which in turn is called from:
> * nbd_eject_notifier(): run in the main thread, not locked
> * nbd_export_remove():
> * qmp_nbd_server_remove(): see above
> * nbd_export_close_all():
> * bdrv_close_all()
> * qmp_nbd_server_stop()

Even weirder: nbd_export_put() calls nbd_export_close(), and
nbd_export_close() calls nbd_export_put().  The mutual recursion is
mind-numbing, and the fact that we use get/put instead of ref/unref like
most other qemu code is not making it any easier to reason about.

> 
> There are also calls from qemu-nbd, but these can be ignored because we
> don't have iothreads there.
> 
> I think the cleanest would be to take the lock in the outermost callers,
> i.e. all QMP handlers that deal with a specific export, in the eject
> notifier and in nbd_export_close_all().

Okay, I'm trying that (I already tried grabbing the aio_context in
nbd_export_close(), but as you predicted, that deadlocked when a nested
call already encountered the lock taken from an outer call).

> 
>> On the other hand, I wonder if there is any situation in which calling
>> to blk_unref() without locking the context could be safe. If there isn't
>> any, perhaps we should assert that the lock is held if blk->ctx != NULL
>> to catch this kind of bugs earlier?
> 
> blk_unref() must be called from the main thread, and if the BlockBackend
> to be unreferenced is not in the main AioContext, the lock must be held.
> 
> I'm not sure how to assert that locks are held, though. I once looked
> for a way to do this, but it involved either looking at the internal
> state of pthreads mutexes or hacking up QemuMutex with debug state.

Even if we can only test that in a debug build but not during normal
builds, could any of our CI builds set up that configuration?

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] nbd/server: attach client channel to the export's AioContext

[Eric Blake]

On 9/12/19 1:37 AM, Sergio Lopez wrote:

>> I tried to test this patch, but even with it applied, I still got an
>> aio-context crasher by attempting an nbd-server-start, nbd-server-add,
>> nbd-server-stop (intentionally skipping the nbd-server-remove step) on a
>> domain using iothreads, with a backtrace of:
>>
>> #0  0x7ff09d070e35 in raise () from target:/lib64/libc.so.6
>> #1  0x7ff09d05b895 in abort () from target:/lib64/libc.so.6
>> #2  0x55dd03b9ab86 in error_exit (err=1, msg=0x55dd03d59fb0
>> <__func__.15769> "qemu_mutex_unlock_impl")
>> at util/qemu-thread-posix.c:36
>> #3  0x55dd03b9adcf in qemu_mutex_unlock_impl (mutex=0x55dd062d5090,
>> file=0x55dd03d59041 "util/async.c",
>> line=523) at util/qemu-thread-posix.c:96
>> #4  0x55dd03b93433 in aio_context_release (ctx=0x55dd062d5030) at
>> util/async.c:523

>> #14 0x55dd03748845 in qmp_nbd_server_stop (errp=0x7ffcdf3cb4e8) at
>> blockdev-nbd.c:233
>> ...

Sorry for truncating the initial stackdump report. The rest of the trace
(it is definitely in the main loop):

#15 0x560be491c910 in qmp_marshal_nbd_server_stop
(args=0x560be54c4d00, ret=0x7ffdd832de38,
errp=0x7ffdd832de30) at qapi/qapi-commands-block.c:318
#16 0x560be4a7a306 in do_qmp_dispatch (cmds=0x560be50dc1f0
, request=0x7fbcac009af0,
allow_oob=false, errp=0x7ffdd832ded8) at qapi/qmp-dispatch.c:131
#17 0x560be4a7a507 in qmp_dispatch (cmds=0x560be50dc1f0
, request=0x7fbcac009af0,
allow_oob=false) at qapi/qmp-dispatch.c:174
#18 0x560be48edd81 in monitor_qmp_dispatch (mon=0x560be55d6670,
req=0x7fbcac009af0) at monitor/qmp.c:120
#19 0x560be48ee116 in monitor_qmp_bh_dispatcher (data=0x0) at
monitor/qmp.c:209
#20 0x560be4ad16a2 in aio_bh_call (bh=0x560be53dbe90) at util/async.c:89
#21 0x560be4ad173a in aio_bh_poll (ctx=0x560be53daba0) at
util/async.c:117
#22 0x560be4ad6514 in aio_dispatch (ctx=0x560be53daba0) at
util/aio-posix.c:459
#23 0x560be4ad1ad3 in aio_ctx_dispatch (source=0x560be53daba0,
callback=0x0, user_data=0x0) at util/async.c:260
#24 0x7fbcd7083ecd in g_main_context_dispatch () from
target:/lib64/libglib-2.0.so.0
#25 0x560be4ad4e47 in glib_pollfds_poll () at util/main-loop.c:218
#26 0x560be4ad4ec1 in os_host_main_loop_wait (timeout=10) at
util/main-loop.c:241
#27 0x560be4ad4fc6 in main_loop_wait (nonblocking=0) at
util/main-loop.c:517
--Type  for more, q to quit, c to continue without paging--
#28 0x560be4691266 in main_loop () at vl.c:1806
#29 0x560be46988a9 in main (argc=112, argv=0x7ffdd832e4e8,
envp=0x7ffdd832e870) at vl.c:4488


>>
>> Does that sound familiar to what you were seeing?  Does it mean we
>> missed another spot where the context is set incorrectly?
> 
> It looks like it was trying to release the AioContext while it was still
> held by some other thread. Is this stacktrace from the main thread or an
> iothread? What was the other one doing?

Kevin had some ideas on what it might be; I'm playing with obtaining the
context in the spots he pointed out.

> 
>> I'm happy to work with you on IRC for more real-time debugging of this
>> (I'm woefully behind on understanding how aio contexts are supposed to
>> work).
> 
> I must be missing some step, because I can't reproduce this one
> here. I've tried both with an idle NDB server and one with a client
> generating I/O. Is it reproducible 100% of them time?

Yes, with iothreads.  I took some time today to boil it down to
something that does not require libvirt:

$ file myfile
myfile: QEMU QCOW2 Image (v3), 104857600 bytes
$ qemu-img create -f qcow2 -o backing_file=myfile,backing_fmt=qcow2  \
 myfile.wrap
Formatting 'myfile.wrap', fmt=qcow2 size=104857600 backing_file=myfile
backing_fmt=qcow2 cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults \
  -name tmp,debug-threads=on -machine pc-q35-3.1,accel=kvm \
  -object iothread,id=iothread1 \
  -drive
file=myfile,format=qcow2,if=none,id=drive-virtio-disk0,node-name=n \
  -device
virtio-blk-pci,iothread=iothread1,drive=drive-virtio-disk0,id=virtio-disk0 \
  -qmp stdio -nographic
{'execute':'qmp_capabilities'}
{'execute':'nbd-server-start','arguments':{'addr':{'type':'inet',
  'data':{'host':'localhost','port':'10809'
{'execute':'blockdev-add','arguments':{'driver':'qcow2',
 'node-name':'t','file'{'driver':'file',
  'filename':'myfile.wrap'},'backing':'n'}}
{'execute':'blockdev-backup','arguments':{'device':'n',
 'target':'t','sync':'none','job-id':'b'}}
{'execute':'nbd-server-add','arguments':{'device':'t','name':'t'}}
{'execute':'nbd-server-remove','arguments':{'name':'t'}}
Aborted (core dumped)

I'm now playing with Kevin's ideas of grabbing the aiocontext around nbd
unref.

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v5 5/5] block/qcow2: introduce parallel subrequest handling in read and write

[Vladimir Sementsov-Ogievskiy]

It improves performance for fragmented qcow2 images.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.h  |   3 ++
 block/qcow2.c  | 125 -
 block/trace-events |   1 +
 3 files changed, 117 insertions(+), 12 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index a488d761ff..f51f478e34 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -65,6 +65,9 @@
 #define QCOW2_MAX_BITMAPS 65535
 #define QCOW2_MAX_BITMAP_DIRECTORY_SIZE (1024 * QCOW2_MAX_BITMAPS)
 
+/* Maximum of parallel sub-request per guest request */
+#define QCOW2_MAX_WORKERS 8
+
 /* indicate that the refcount of the referenced cluster is exactly one. */
 #define QCOW_OFLAG_COPIED (1ULL << 63)
 /* indicate that the cluster is compressed (they never have the copied flag) */
diff --git a/block/qcow2.c b/block/qcow2.c
index 164b22e4a2..7961c05783 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -41,6 +41,7 @@
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qapi-visit-block-core.h"
 #include "crypto.h"
+#include "block/aio_task.h"
 
 /*
   Differences with QCOW:
@@ -2025,6 +2026,60 @@ fail:
 return ret;
 }
 
+typedef struct Qcow2AioTask {
+AioTask task;
+
+BlockDriverState *bs;
+QCow2ClusterType cluster_type; /* only for read */
+uint64_t file_cluster_offset;
+uint64_t offset;
+uint64_t bytes;
+QEMUIOVector *qiov;
+uint64_t qiov_offset;
+QCowL2Meta *l2meta; /* only for write */
+} Qcow2AioTask;
+
+static coroutine_fn int qcow2_co_preadv_task_entry(AioTask *task);
+static coroutine_fn int qcow2_add_task(BlockDriverState *bs,
+   AioTaskPool *pool,
+   AioTaskFunc func,
+   QCow2ClusterType cluster_type,
+   uint64_t file_cluster_offset,
+   uint64_t offset,
+   uint64_t bytes,
+   QEMUIOVector *qiov,
+   size_t qiov_offset,
+   QCowL2Meta *l2meta)
+{
+Qcow2AioTask local_task;
+Qcow2AioTask *task = pool ? g_new(Qcow2AioTask, 1) : &local_task;
+
+*task = (Qcow2AioTask) {
+.task.func = func,
+.bs = bs,
+.cluster_type = cluster_type,
+.qiov = qiov,
+.file_cluster_offset = file_cluster_offset,
+.offset = offset,
+.bytes = bytes,
+.qiov_offset = qiov_offset,
+.l2meta = l2meta,
+};
+
+trace_qcow2_add_task(qemu_coroutine_self(), bs, pool,
+ func == qcow2_co_preadv_task_entry ? "read" : "write",
+ cluster_type, file_cluster_offset, offset, bytes,
+ qiov, qiov_offset);
+
+if (!pool) {
+return func(&task->task);
+}
+
+aio_task_pool_start_task(pool, &task->task);
+
+return 0;
+}
+
 static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
  QCow2ClusterType cluster_type,
  uint64_t file_cluster_offset,
@@ -2074,18 +2129,28 @@ static coroutine_fn int 
qcow2_co_preadv_task(BlockDriverState *bs,
 g_assert_not_reached();
 }
 
+static coroutine_fn int qcow2_co_preadv_task_entry(AioTask *task)
+{
+Qcow2AioTask *t = container_of(task, Qcow2AioTask, task);
+
+assert(!t->l2meta);
+
+return qcow2_co_preadv_task(t->bs, t->cluster_type, t->file_cluster_offset,
+t->offset, t->bytes, t->qiov, t->qiov_offset);
+}
+
 static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
  uint64_t offset, uint64_t bytes,
  QEMUIOVector *qiov,
  size_t qiov_offset, int flags)
 {
 BDRVQcow2State *s = bs->opaque;
-int ret;
+int ret = 0;
 unsigned int cur_bytes; /* number of bytes in current iteration */
 uint64_t cluster_offset = 0;
+AioTaskPool *aio = NULL;
 
-while (bytes != 0) {
-
+while (bytes != 0 && aio_task_pool_status(aio) == 0) {
 /* prepare next request */
 cur_bytes = MIN(bytes, INT_MAX);
 if (s->crypto) {
@@ -2097,7 +2162,7 @@ static coroutine_fn int 
qcow2_co_preadv_part(BlockDriverState *bs,
 ret = qcow2_get_cluster_offset(bs, offset, &cur_bytes, 
&cluster_offset);
 qemu_co_mutex_unlock(&s->lock);
 if (ret < 0) {
-return ret;
+goto out;
 }
 
 if (ret == QCOW2_CLUSTER_ZERO_PLAIN ||
@@ -2106,11 +2171,14 @@ static coroutine_fn int 
qcow2_co_preadv_part(BlockDriverState *bs,
 {
 qemu_iovec_memset(qiov, qiov_offset, 0, cur_bytes);
 } else {
-ret = qcow2_co_preadv_task(bs, ret,
-   cluster_of

[Qemu-block] [PATCH v5 0/5] qcow2: async handling of fragmented io

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is an asynchronous scheme for handling fragmented qcow2
reads and writes. Both qcow2 read and write functions loops through
sequential portions of data. The series aim it to parallelize these
loops iterations.
It improves performance for fragmented qcow2 images, I've tested it
as described below.

v5: fix 026 and rebase on Max's block branch [perf results not updated]:

01: new, prepare 026 to not fail
03: - drop read_encrypted blkdbg event [Kevin]
- assert((x & (BDRV_SECTOR_SIZE - 1)) == 0) -> assert(QEMU_IS_ALIGNED(x, 
BDRV_SECTOR_SIZE)) [rebase]
- full host offset in argument of qcow2_co_decrypt [rebase]
04: - substitute remaining qcow2_co_do_pwritev by qcow2_co_pwritev_task in 
comment [Max]
- full host offset in argument of qcow2_co_encrypt [rebase]
05: - Now patch don't affect 026 iotest, so its output is not changed

Rebase changes seems trivial, so, I've kept r-b marks.

Based-on: https://github.com/XanClic/qemu.git block

About testing:

I have four 4G qcow2 images (with default 64k block size) on my ssd disk:
t-seq.qcow2 - sequentially written qcow2 image
t-reverse.qcow2 - filled by writing 64k portions from end to the start
t-rand.qcow2 - filled by writing 64k portions (aligned) in random order
t-part-rand.qcow2 - filled by shuffling order of 64k writes in 1m clusters
(see source code of image generation in the end for details)

and I've done several runs like the following (sequential io by 1mb chunks):

out=/tmp/block; echo > $out; cat /tmp/files | while read file; do for wr in 
{"","-w"}; do echo "$file" $wr; ./qemu-img bench -c 4096 -d 1 -f qcow2 -n -s 1m 
-t none $wr "$file" | grep 'Run completed in' | awk '{print $4}' >> $out; done; 
done


short info about parameters:
  -w - do writes (otherwise do reads)
  -c - count of blocks
  -s - block size
  -t none - disable cache
  -n - native aio
  -d 1 - don't use parallel requests provided by qemu-img bench itself

results:

+---+-+-+
| file  | master  | async   |
+---+-+-+
| /ssd/t-part-rand.qcow2| 14.671  | 9.193   |
+---+-+-+
| /ssd/t-part-rand.qcow2 -w | 11.434  | 8.621   |
+---+-+-+
| /ssd/t-rand.qcow2 | 20.421  | 10.05   |
+---+-+-+
| /ssd/t-rand.qcow2 -w  | 11.097  | 8.915   |
+---+-+-+
| /ssd/t-reverse.qcow2  | 17.515  | 9.407   |
+---+-+-+
| /ssd/t-reverse.qcow2 -w   | 11.255  | 8.649   |
+---+-+-+
| /ssd/t-seq.qcow2  | 9.081   | 9.072   |
+---+-+-+
| /ssd/t-seq.qcow2 -w   | 8.761   | 8.747   |
+---+-+-+
| /tmp/t-part-rand.qcow2| 41.179  | 41.37   |
+---+-+-+
| /tmp/t-part-rand.qcow2 -w | 54.097  | 55.323  |
+---+-+-+
| /tmp/t-rand.qcow2 | 711.899 | 514.339 |
+---+-+-+
| /tmp/t-rand.qcow2 -w  | 546.259 | 642.114 |
+---+-+-+
| /tmp/t-reverse.qcow2  | 86.065  | 96.522  |
+---+-+-+
| /tmp/t-reverse.qcow2 -w   | 46.557  | 48.499  |
+---+-+-+
| /tmp/t-seq.qcow2  | 33.804  | 33.862  |
+---+-+-+
| /tmp/t-seq.qcow2 -w   | 34.299  | 34.233  |
+---+-+-+


Performance gain is obvious, especially for read and especially for ssd.
For hdd there is a degradation for reverse case, but this is the most
impossible case and seems not critical.

How images are generated:

 gen-writes ==
#!/usr/bin/env python
import random
import sys

size = 4 * 1024 * 1024 * 1024
block = 64 * 1024
block2 = 1024 * 1024

arg = sys.argv[1]

if arg in ('rand', 'reverse', 'seq'):
writes = list(range(0, size, block))

if arg == 'rand':
random.shuffle(writes)
elif arg == 'reverse':
writes.reverse()
elif arg == 'part-rand':
writes = []
for off in range(0, size, block2):
wr = list(range(off, off + block2, block))
random.shuffle(wr)
writes.extend(wr)
elif arg != 'seq':
sys.exit(1)

for w in writes:
print 'write -P 0xff {} {}'.format(w, block)

print 'q'
==

= gen-test-images.sh =
#!/bin/bash

IMG_PATH=/ssd

for name in seq reverse rand part-rand; do
IMG=$IMG_PATH/t-$name.qcow2
echo createing $IMG ...
  

[Qemu-block] [PATCH v5 1/5] qemu-iotests: ignore leaks on failure paths in 026

[Vladimir Sementsov-Ogievskiy]

Upcoming asynchronous handling of sub-parts of qcow2 requests will
change number of leaked clusters and even make it racy. As a
preparation, ignore leaks on failure parts in 026.

It's not trivial to just grep or substitute qemu-img output for such
thing. Instead do better: 3 is a error code of qemu-img check, if only
leaks are found. Catch this case and print success output.

Suggested-by: Anton Nefedov 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/026 |  6 +--
 tests/qemu-iotests/026.out | 80 --
 tests/qemu-iotests/026.out.nocache | 80 --
 tests/qemu-iotests/common.rc   | 17 +++
 4 files changed, 60 insertions(+), 123 deletions(-)

diff --git a/tests/qemu-iotests/026 b/tests/qemu-iotests/026
index ffb18ab6b5..3430029ed6 100755
--- a/tests/qemu-iotests/026
+++ b/tests/qemu-iotests/026
@@ -107,7 +107,7 @@ if [ "$event" == "l2_load" ]; then
 $QEMU_IO -c "read $vmstate 0 128k " "$BLKDBG_TEST_IMG" | _filter_qemu_io
 fi
 
-_check_test_img 2>&1 | grep -v "refcount=1 reference=0"
+_check_test_img_ignore_leaks 2>&1 | grep -v "refcount=1 reference=0"
 
 done
 done
@@ -152,7 +152,7 @@ echo
 echo "Event: $event; errno: $errno; imm: $imm; once: $once; write $vmstate"
 $QEMU_IO -c "write $vmstate 0 64M" "$BLKDBG_TEST_IMG" | _filter_qemu_io
 
-_check_test_img 2>&1 | grep -v "refcount=1 reference=0"
+_check_test_img_ignore_leaks 2>&1 | grep -v "refcount=1 reference=0"
 
 done
 done
@@ -191,7 +191,7 @@ echo
 echo "Event: $event; errno: $errno; imm: $imm; once: $once"
 $QEMU_IO -c "write -b 0 64k" "$BLKDBG_TEST_IMG" | _filter_qemu_io
 
-_check_test_img 2>&1 | grep -v "refcount=1 reference=0"
+_check_test_img_ignore_leaks 2>&1 | grep -v "refcount=1 reference=0"
 
 done
 done
diff --git a/tests/qemu-iotests/026.out b/tests/qemu-iotests/026.out
index fb89b8480c..ff0817b6f2 100644
--- a/tests/qemu-iotests/026.out
+++ b/tests/qemu-iotests/026.out
@@ -17,18 +17,14 @@ Event: l1_update; errno: 5; imm: off; once: off; write
 qemu-io: Failed to flush the L2 table cache: Input/output error
 qemu-io: Failed to flush the refcount block cache: Input/output error
 write failed: Input/output error
-
-1 leaked clusters were found on the image.
-This means waste of disk space, but no harm to data.
+No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l1_update; errno: 5; imm: off; once: off; write -b
 qemu-io: Failed to flush the L2 table cache: Input/output error
 qemu-io: Failed to flush the refcount block cache: Input/output error
 write failed: Input/output error
-
-1 leaked clusters were found on the image.
-This means waste of disk space, but no harm to data.
+No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l1_update; errno: 28; imm: off; once: on; write
@@ -45,18 +41,14 @@ Event: l1_update; errno: 28; imm: off; once: off; write
 qemu-io: Failed to flush the L2 table cache: No space left on device
 qemu-io: Failed to flush the refcount block cache: No space left on device
 write failed: No space left on device
-
-1 leaked clusters were found on the image.
-This means waste of disk space, but no harm to data.
+No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l1_update; errno: 28; imm: off; once: off; write -b
 qemu-io: Failed to flush the L2 table cache: No space left on device
 qemu-io: Failed to flush the refcount block cache: No space left on device
 write failed: No space left on device
-
-1 leaked clusters were found on the image.
-This means waste of disk space, but no harm to data.
+No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_load; errno: 5; imm: off; once: on; write
@@ -137,18 +129,14 @@ Event: l2_update; errno: 5; imm: off; once: off; write
 qemu-io: Failed to flush the L2 table cache: Input/output error
 qemu-io: Failed to flush the refcount block cache: Input/output error
 write failed: Input/output error
-
-127 leaked clusters were found on the image.
-This means waste of disk space, but no harm to data.
+No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_update; errno: 5; imm: off; once: off; write -b
 qemu-io: Failed to flush the L2 table cache: Input/output error
 qemu-io: Failed to flush the refcount block cache: Input/output error
 write failed: Input/output error
-
-127 leaked clusters were found on the image.
-This means waste of disk space, but no harm to data.
+No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_update; errno: 28; imm: off; once: on; write
@@ -165,18 +153,14 @@ Event: l2_update; errno: 28; imm: off; once: off; write
 qemu-io: Failed to flush the L2 table cache: No space left on device
 qemu-io: Failed to flush the refcount block cache: No space left on

[Qemu-block] [PATCH v5 4/5] block/qcow2: refactor qcow2_co_pwritev_part

[Vladimir Sementsov-Ogievskiy]

Similarly to previous commit, prepare for parallelizing write-loop
iterations.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/qcow2.c | 154 +-
 1 file changed, 90 insertions(+), 64 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 6feb169f7c..164b22e4a2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2242,6 +2242,88 @@ static int handle_alloc_space(BlockDriverState *bs, 
QCowL2Meta *l2meta)
 return 0;
 }
 
+/*
+ * qcow2_co_pwritev_task
+ * Called with s->lock unlocked
+ * l2meta  - if not NULL, qcow2_co_pwritev_task() will consume it. Caller must
+ *   not use it somehow after qcow2_co_pwritev_task() call
+ */
+static coroutine_fn int qcow2_co_pwritev_task(BlockDriverState *bs,
+  uint64_t file_cluster_offset,
+  uint64_t offset, uint64_t bytes,
+  QEMUIOVector *qiov,
+  uint64_t qiov_offset,
+  QCowL2Meta *l2meta)
+{
+int ret;
+BDRVQcow2State *s = bs->opaque;
+void *crypt_buf = NULL;
+int offset_in_cluster = offset_into_cluster(s, offset);
+QEMUIOVector encrypted_qiov;
+
+if (bs->encrypted) {
+assert(s->crypto);
+assert(bytes <= QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
+crypt_buf = qemu_try_blockalign(bs->file->bs, bytes);
+if (crypt_buf == NULL) {
+ret = -ENOMEM;
+goto out_unlocked;
+}
+qemu_iovec_to_buf(qiov, qiov_offset, crypt_buf, bytes);
+
+if (qcow2_co_encrypt(bs, file_cluster_offset + offset_in_cluster,
+ offset, crypt_buf, bytes) < 0)
+{
+ret = -EIO;
+goto out_unlocked;
+}
+
+qemu_iovec_init_buf(&encrypted_qiov, crypt_buf, bytes);
+qiov = &encrypted_qiov;
+qiov_offset = 0;
+}
+
+/* Try to efficiently initialize the physical space with zeroes */
+ret = handle_alloc_space(bs, l2meta);
+if (ret < 0) {
+goto out_unlocked;
+}
+
+/*
+ * If we need to do COW, check if it's possible to merge the
+ * writing of the guest data together with that of the COW regions.
+ * If it's not possible (or not necessary) then write the
+ * guest data now.
+ */
+if (!merge_cow(offset, bytes, qiov, qiov_offset, l2meta)) {
+BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
+trace_qcow2_writev_data(qemu_coroutine_self(),
+file_cluster_offset + offset_in_cluster);
+ret = bdrv_co_pwritev_part(s->data_file,
+   file_cluster_offset + offset_in_cluster,
+   bytes, qiov, qiov_offset, 0);
+if (ret < 0) {
+goto out_unlocked;
+}
+}
+
+qemu_co_mutex_lock(&s->lock);
+
+ret = qcow2_handle_l2meta(bs, &l2meta, true);
+goto out_locked;
+
+out_unlocked:
+qemu_co_mutex_lock(&s->lock);
+
+out_locked:
+qcow2_handle_l2meta(bs, &l2meta, false);
+qemu_co_mutex_unlock(&s->lock);
+
+qemu_vfree(crypt_buf);
+
+return ret;
+}
+
 static coroutine_fn int qcow2_co_pwritev_part(
 BlockDriverState *bs, uint64_t offset, uint64_t bytes,
 QEMUIOVector *qiov, size_t qiov_offset, int flags)
@@ -2251,15 +2333,10 @@ static coroutine_fn int qcow2_co_pwritev_part(
 int ret;
 unsigned int cur_bytes; /* number of sectors in current iteration */
 uint64_t cluster_offset;
-QEMUIOVector encrypted_qiov;
-uint64_t bytes_done = 0;
-uint8_t *cluster_data = NULL;
 QCowL2Meta *l2meta = NULL;
 
 trace_qcow2_writev_start_req(qemu_coroutine_self(), offset, bytes);
 
-qemu_co_mutex_lock(&s->lock);
-
 while (bytes != 0) {
 
 l2meta = NULL;
@@ -2273,6 +2350,8 @@ static coroutine_fn int qcow2_co_pwritev_part(
 - offset_in_cluster);
 }
 
+qemu_co_mutex_lock(&s->lock);
+
 ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
  &cluster_offset, &l2meta);
 if (ret < 0) {
@@ -2290,73 +2369,20 @@ static coroutine_fn int qcow2_co_pwritev_part(
 
 qemu_co_mutex_unlock(&s->lock);
 
-if (bs->encrypted) {
-assert(s->crypto);
-if (!cluster_data) {
-cluster_data = qemu_try_blockalign(bs->file->bs,
-   QCOW_MAX_CRYPT_CLUSTERS
-   * s->cluster_size);
-if (cluster_data == NULL) {
-ret = -ENOMEM;
-goto out_unlocked;
-}
-}
-
-assert(cur_bytes <= QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
-qemu_iovec_to_buf(qiov, qiov_offset + bytes_d

[Qemu-block] [PATCH v5 3/5] block/qcow2: refactor qcow2_co_preadv_part

[Vladimir Sementsov-Ogievskiy]

Further patch will run partial requests of iterations of
qcow2_co_preadv in parallel for performance reasons. To prepare for
this, separate part which may be parallelized into separate function
(qcow2_co_preadv_task).

While being here, also separate encrypted clusters reading to own
function, like it is done for compressed reading.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/qcow2.c | 209 +++---
 1 file changed, 113 insertions(+), 96 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 4d16393e61..6feb169f7c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1972,17 +1972,117 @@ out:
 return ret;
 }
 
+static coroutine_fn int
+qcow2_co_preadv_encrypted(BlockDriverState *bs,
+   uint64_t file_cluster_offset,
+   uint64_t offset,
+   uint64_t bytes,
+   QEMUIOVector *qiov,
+   uint64_t qiov_offset)
+{
+int ret;
+BDRVQcow2State *s = bs->opaque;
+uint8_t *buf;
+
+assert(bs->encrypted && s->crypto);
+assert(bytes <= QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
+
+/*
+ * For encrypted images, read everything into a temporary
+ * contiguous buffer on which the AES functions can work.
+ * Also, decryption in a separate buffer is better as it
+ * prevents the guest from learning information about the
+ * encrypted nature of the virtual disk.
+ */
+
+buf = qemu_try_blockalign(s->data_file->bs, bytes);
+if (buf == NULL) {
+return -ENOMEM;
+}
+
+BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
+ret = bdrv_co_pread(s->data_file,
+file_cluster_offset + offset_into_cluster(s, offset),
+bytes, buf, 0);
+if (ret < 0) {
+goto fail;
+}
+
+assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
+assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
+if (qcow2_co_decrypt(bs,
+ file_cluster_offset + offset_into_cluster(s, offset),
+ offset, buf, bytes) < 0)
+{
+ret = -EIO;
+goto fail;
+}
+qemu_iovec_from_buf(qiov, qiov_offset, buf, bytes);
+
+fail:
+qemu_vfree(buf);
+
+return ret;
+}
+
+static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
+ QCow2ClusterType cluster_type,
+ uint64_t file_cluster_offset,
+ uint64_t offset, uint64_t bytes,
+ QEMUIOVector *qiov,
+ size_t qiov_offset)
+{
+BDRVQcow2State *s = bs->opaque;
+int offset_in_cluster = offset_into_cluster(s, offset);
+
+switch (cluster_type) {
+case QCOW2_CLUSTER_ZERO_PLAIN:
+case QCOW2_CLUSTER_ZERO_ALLOC:
+/* Both zero types are handled in qcow2_co_preadv_part */
+g_assert_not_reached();
+
+case QCOW2_CLUSTER_UNALLOCATED:
+assert(bs->backing); /* otherwise handled in qcow2_co_preadv_part */
+
+BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
+return bdrv_co_preadv_part(bs->backing, offset, bytes,
+   qiov, qiov_offset, 0);
+
+case QCOW2_CLUSTER_COMPRESSED:
+return qcow2_co_preadv_compressed(bs, file_cluster_offset,
+  offset, bytes, qiov, qiov_offset);
+
+case QCOW2_CLUSTER_NORMAL:
+if ((file_cluster_offset & 511) != 0) {
+return -EIO;
+}
+
+if (bs->encrypted) {
+return qcow2_co_preadv_encrypted(bs, file_cluster_offset,
+ offset, bytes, qiov, qiov_offset);
+}
+
+BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
+return bdrv_co_preadv_part(s->data_file,
+   file_cluster_offset + offset_in_cluster,
+   bytes, qiov, qiov_offset, 0);
+
+default:
+g_assert_not_reached();
+}
+
+g_assert_not_reached();
+}
+
 static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
  uint64_t offset, uint64_t bytes,
  QEMUIOVector *qiov,
  size_t qiov_offset, int flags)
 {
 BDRVQcow2State *s = bs->opaque;
-int offset_in_cluster;
 int ret;
 unsigned int cur_bytes; /* number of bytes in current iteration */
 uint64_t cluster_offset = 0;
-uint8_t *cluster_data = NULL;
 
 while (bytes != 0) {
 
@@ -1997,112 +2097,29 @@ static coroutine_fn int 
qcow2_co_preadv_part(BlockDriverState *bs,
 ret = qcow2_get_cluster_offset(bs, offset, &cur_bytes, 
&cluster_offset);
 qemu_co_mutex_unlock(&s->lock);
 if (ret < 0) {
-   

[Qemu-block] [PATCH v5 2/5] block: introduce aio task pool

[Vladimir Sementsov-Ogievskiy]

Common interface for aio task loops. To be used for improving
performance of synchronous io loops in qcow2, block-stream,
copy-on-read, and may be other places.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 include/block/aio_task.h |  54 +
 block/aio_task.c | 124 +++
 block/Makefile.objs  |   2 +
 3 files changed, 180 insertions(+)
 create mode 100644 include/block/aio_task.h
 create mode 100644 block/aio_task.c

diff --git a/include/block/aio_task.h b/include/block/aio_task.h
new file mode 100644
index 00..50bc1e1817
--- /dev/null
+++ b/include/block/aio_task.h
@@ -0,0 +1,54 @@
+/*
+ * Aio tasks loops
+ *
+ * Copyright (c) 2019 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 BLOCK_AIO_TASK_H
+#define BLOCK_AIO_TASK_H
+
+#include "qemu/coroutine.h"
+
+typedef struct AioTaskPool AioTaskPool;
+typedef struct AioTask AioTask;
+typedef int coroutine_fn (*AioTaskFunc)(AioTask *task);
+struct AioTask {
+AioTaskPool *pool;
+AioTaskFunc func;
+int ret;
+};
+
+AioTaskPool *coroutine_fn aio_task_pool_new(int max_busy_tasks);
+void aio_task_pool_free(AioTaskPool *);
+
+/* error code of failed task or 0 if all is OK */
+int aio_task_pool_status(AioTaskPool *pool);
+
+bool aio_task_pool_empty(AioTaskPool *pool);
+
+/* User provides filled @task, however task->pool will be set automatically */
+void coroutine_fn aio_task_pool_start_task(AioTaskPool *pool, AioTask *task);
+
+void coroutine_fn aio_task_pool_wait_slot(AioTaskPool *pool);
+void coroutine_fn aio_task_pool_wait_one(AioTaskPool *pool);
+void coroutine_fn aio_task_pool_wait_all(AioTaskPool *pool);
+
+#endif /* BLOCK_AIO_TASK_H */
diff --git a/block/aio_task.c b/block/aio_task.c
new file mode 100644
index 00..88989fa248
--- /dev/null
+++ b/block/aio_task.c
@@ -0,0 +1,124 @@
+/*
+ * Aio tasks loops
+ *
+ * Copyright (c) 2019 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 "block/aio.h"
+#include "block/aio_task.h"
+
+struct AioTaskPool {
+Coroutine *main_co;
+int status;
+int max_busy_tasks;
+int busy_tasks;
+bool waiting;
+};
+
+static void coroutine_fn aio_task_co(void *opaque)
+{
+AioTask *task = opaque;
+AioTaskPool *pool = task->pool;
+
+assert(pool->busy_tasks < pool->max_busy_tasks);
+pool->busy_tasks++;
+
+task->ret = task->func(task);
+
+pool->busy_tasks--;
+
+if (task->ret < 0 && pool->status == 0) {
+pool->status = task->ret;
+}
+
+g_free(task);
+
+if (pool->waiting) {
+pool->waiting = false;
+aio_co_wake(pool->main_co);
+}
+}
+
+void coroutine_fn aio_task_pool_wait_one(AioTaskPool *pool)
+{
+assert(pool->busy_tasks > 0);
+assert(qemu_coroutine_self() == pool->main_co);

Re: [Qemu-block] [Qemu-devel] [PATCH 1/4] block/dirty-bitmap: drop meta

[John Snow]

On 9/16/19 10:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> Drop meta bitmaps, as they are unused.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

believe it or not, I had a local patch that does the same :)

Reviewed-by: John Snow 

Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/4] iotests: add script_initialize

[John Snow]

On 9/16/19 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> 16.09.2019 19:32, John Snow wrote:
>>
>>
>> On 9/16/19 10:56 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 12.09.2019 3:16, John Snow wrote:
 Like script_main, but doesn't require a single point of entry.
 Replace all existing initialization sections with this drop-in replacement.

 This brings debug support to all existing script-style iotests.

 Note: supported_oses=['linux'] was omitted, as it is a default argument.
>>>
>>> But after this patch all test which didn't check os start to check linux
>>> (as it's default).. So all tests which worked on other platforms will now
>>> be skipped on these other platforms?
>>>
>>
>> def verify_platform(supported_oses=['linux']):
>>  if True not in [sys.platform.startswith(x) for x in supported_oses]:
>>  notrun('not suitable for this OS: %s' % sys.platform)
>>
>>
>> It was already the default. I didn't *make* it the default.
> 
> Yes. But for some tests, verify_platform wasn't called before this patch at 
> all.
> And now it is called and checks "linux". It's the change. Or what I miss?
> 

I guess there are more than I thought.
(I thought it was just one, based on a discussion with Philippe.)

So, you're right: there are more tests now that will refuse to run on
non-Linux platforms.

Those tests are:
206
207
208
209
210
211
212
213
218
219
235
236
237
238
242
246
248
254
255
256

What sets these apart from the other python-based tests? Nothing in
particular? Just depends on who copied from whom that day. I don't think
there's any rhyme or reason to any of it.

I don't really have a BSD or OSX machine to test which ones should and
should not be limited, either.

Kevin, do you remember why we added these checks?

--js

Re: [Qemu-block] [PATCH v2 1/2] trace: Remove trailing newline in events

[John Snow]

On 9/16/19 12:40 PM, Philippe Mathieu-Daudé wrote:
> On 9/16/19 6:36 PM, Eric Blake wrote:
>> On 9/16/19 4:51 AM, Philippe Mathieu-Daudé wrote:
>>> While the tracing frawework does not forbid trailing newline in
>>
>> framework
>>
>>> events format string, using them lead to confuse output.
>>> It is the responsibility of the backend to properly end an event
>>> line.
>>
>> Why just trailing newline? Should we not forbid ALL use of newline in a
>> trace message?
> 
> I thought about it and forgot to add a comment when respining.
> Yes, I think this is the right thing to enforce.
> However it requires more cleanup, affecting more subsystems, so I'd
> rather keep it for a follow-up series.

What's the problem with using multi-line trace statements?
Does it interfere with non-printf backends?

(I think I intentionally use these for AHCI in a few places.)

--js

Re: [Qemu-block] [PATCH v2 1/2] trace: Remove trailing newline in events

[Philippe Mathieu-Daudé]

On 9/16/19 6:36 PM, Eric Blake wrote:
> On 9/16/19 4:51 AM, Philippe Mathieu-Daudé wrote:
>> While the tracing frawework does not forbid trailing newline in
> 
> framework
> 
>> events format string, using them lead to confuse output.
>> It is the responsibility of the backend to properly end an event
>> line.
> 
> Why just trailing newline? Should we not forbid ALL use of newline in a
> trace message?

I thought about it and forgot to add a comment when respining.
Yes, I think this is the right thing to enforce.
However it requires more cleanup, affecting more subsystems, so I'd
rather keep it for a follow-up series.

>>
>> Some of our formats have trailing newlines, remove them.
>>
>> Reviewed-by: John Snow 
>> Reviewed-by: Kevin Wolf 
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---

Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/4] iotests: add script_initialize

[Vladimir Sementsov-Ogievskiy]

16.09.2019 19:32, John Snow wrote:
> 
> 
> On 9/16/19 10:56 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 12.09.2019 3:16, John Snow wrote:
>>> Like script_main, but doesn't require a single point of entry.
>>> Replace all existing initialization sections with this drop-in replacement.
>>>
>>> This brings debug support to all existing script-style iotests.
>>>
>>> Note: supported_oses=['linux'] was omitted, as it is a default argument.
>>
>> But after this patch all test which didn't check os start to check linux
>> (as it's default).. So all tests which worked on other platforms will now
>> be skipped on these other platforms?
>>
> 
> def verify_platform(supported_oses=['linux']):
>  if True not in [sys.platform.startswith(x) for x in supported_oses]:
>  notrun('not suitable for this OS: %s' % sys.platform)
> 
> 
> It was already the default. I didn't *make* it the default.

Yes. But for some tests, verify_platform wasn't called before this patch at all.
And now it is called and checks "linux". It's the change. Or what I miss?

> There is no
> change here. Feel free to propose a fix, but I don't think it's within
> the scope of this series.
> 
>> Finally do we support something except linux for iotests?
>> for bash tests _supported_os also used only with "Linux" in 87 tests..
>>
> 
> Evidently, not really. See bc521696607c5348fcd8a9e57b408d0ac0dbe2f8
> 
>> May be we'd better drop both _supported_os and supported_oses alltogether,
>> and don't care?
>>
> 
> Beyond the scope of this series.
> 
>> Anyway, if we support only linux, any reason to skip almost all tests,
>> if someone tries to run test on other platform? Let him run what he wants.
> 
> Maybe true, maybe not.
> 
>>
>>> Signed-off-by: John Snow 
>>> ---
>>
>> [..]
>>
>>> +def execute_test(test_function=None, *args, **kwargs):
>>> +"""Run either unittest or script-style tests."""
>>> +
>>> +debug = execute_setup_common(*args, **kwargs)
>>>if not test_function:
>>> -execute_unittest(output, verbosity, debug)
>>> +execute_unittest(debug)
>>>else:
>>>test_function()
>>>
>>> +# This is called from script-style iotests without a single point of entry
>>> +def script_initialize(*args, **kwargs):
>>> +"""Initialize script-style tests without running any tests."""
>>> +execute_setup_common(*args, **kwargs)
>>> +
>>> +# This is called from script-style iotests with a single point of entry
>>>def script_main(test_function, *args, **kwargs):
>>>"""Run script-style tests outside of the unittest framework"""
>>>execute_test(test_function, *args, **kwargs)
>>>
>>> +# This is called from unittest style iotests
>>>def main(*args, **kwargs):
>>>"""Run tests using the unittest framework"""
>>
>>
>> Hmm, now two different styles of code documenting used: comment and 
>> doc-string,
>> both containing almost equal meaning.. I don't like it, still don't really 
>> mind.
> 
> I think I was trying to document what the function /does/ separately
> from a note about explaining how and where it is used. It's quite nearly
> redundant and if it's distracting I can remove it.
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PATCH v2] util/hbitmap: strict hbitmap_reset

[John Snow]

On 9/16/19 4:00 AM, Kevin Wolf wrote:
> Am 13.09.2019 um 20:49 hat John Snow geschrieben:
>> On 9/12/19 4:20 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Also, I'm not sure about "are" suggested by Max. "are" is for plural, but 
>>> here I meant
>>> one object: sum of @start and @count.
>>>
>>
>> There's not great agreement universally about how to treat things like
>> collective nouns. Sometimes "Data" is singular, but sometimes it's
>> plural. "It depends."
>>
>> In this case, "start + count" refers to one sum, but two constituent
>> pieces, so it's functioning like a collective noun.
>>
>> We might say "a + b (together) /are/ ..." but also "the sum of a + b /is/".
>>
>>> So, you may use exactly "Sum of @start and @count is" or "(@start + @count) 
>>> sum is" or
>>> just "(@start + @count) is", whichever you like more.
>>>
>>
>> I like using "the sum of @x and @y is" for being grammatically unambiguous.
>>
>> updated and pushed.
>>
>> (Sorry about my language again! --js)
> 
> Time to revive https://patchwork.kernel.org/patch/8725621/? ;-)
> 
> Kevin
> 

Ja, bitte.

Re: [Qemu-block] [PATCH v2 1/2] trace: Remove trailing newline in events

[Eric Blake]

On 9/16/19 4:51 AM, Philippe Mathieu-Daudé wrote:
> While the tracing frawework does not forbid trailing newline in

framework

> events format string, using them lead to confuse output.
> It is the responsibility of the backend to properly end an event
> line.

Why just trailing newline? Should we not forbid ALL use of newline in a
trace message?

> 
> Some of our formats have trailing newlines, remove them.
> 
> Reviewed-by: John Snow 
> Reviewed-by: Kevin Wolf 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---


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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/4] iotests: add script_initialize

[John Snow]

On 9/16/19 10:56 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.09.2019 3:16, John Snow wrote:
>> Like script_main, but doesn't require a single point of entry.
>> Replace all existing initialization sections with this drop-in replacement.
>>
>> This brings debug support to all existing script-style iotests.
>>
>> Note: supported_oses=['linux'] was omitted, as it is a default argument.
> 
> But after this patch all test which didn't check os start to check linux
> (as it's default).. So all tests which worked on other platforms will now
> be skipped on these other platforms?
> 

def verify_platform(supported_oses=['linux']):
if True not in [sys.platform.startswith(x) for x in supported_oses]:
notrun('not suitable for this OS: %s' % sys.platform)


It was already the default. I didn't *make* it the default. There is no
change here. Feel free to propose a fix, but I don't think it's within
the scope of this series.

> Finally do we support something except linux for iotests?
> for bash tests _supported_os also used only with "Linux" in 87 tests..
> 

Evidently, not really. See bc521696607c5348fcd8a9e57b408d0ac0dbe2f8

> May be we'd better drop both _supported_os and supported_oses alltogether,
> and don't care?
> 

Beyond the scope of this series.

> Anyway, if we support only linux, any reason to skip almost all tests,
> if someone tries to run test on other platform? Let him run what he wants.

Maybe true, maybe not.

> 
>> Signed-off-by: John Snow 
>> ---
> 
> [..]
> 
>> +def execute_test(test_function=None, *args, **kwargs):
>> +"""Run either unittest or script-style tests."""
>> +
>> +debug = execute_setup_common(*args, **kwargs)
>>   if not test_function:
>> -execute_unittest(output, verbosity, debug)
>> +execute_unittest(debug)
>>   else:
>>   test_function()
>>   
>> +# This is called from script-style iotests without a single point of entry
>> +def script_initialize(*args, **kwargs):
>> +"""Initialize script-style tests without running any tests."""
>> +execute_setup_common(*args, **kwargs)
>> +
>> +# This is called from script-style iotests with a single point of entry
>>   def script_main(test_function, *args, **kwargs):
>>   """Run script-style tests outside of the unittest framework"""
>>   execute_test(test_function, *args, **kwargs)
>>   
>> +# This is called from unittest style iotests
>>   def main(*args, **kwargs):
>>   """Run tests using the unittest framework"""
> 
> 
> Hmm, now two different styles of code documenting used: comment and 
> doc-string,
> both containing almost equal meaning.. I don't like it, still don't really 
> mind.

I think I was trying to document what the function /does/ separately
from a note about explaining how and where it is used. It's quite nearly
redundant and if it's distracting I can remove it.

Re: [Qemu-block] [PATCH v4 0/5] qcow2: async handling of fragmented io

[Vladimir Sementsov-Ogievskiy]

16.09.2019 16:26, Max Reitz wrote:
> On 13.09.19 10:58, Max Reitz wrote:
>> On 16.08.19 17:30, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> Here is an asynchronous scheme for handling fragmented qcow2
>>> reads and writes. Both qcow2 read and write functions loops through
>>> sequential portions of data. The series aim it to parallelize these
>>> loops iterations.
>>> It improves performance for fragmented qcow2 images, I've tested it
>>> as described below.
>>
>> Thanks, I’ve changed two things:
>> - Replaced assert((x & (BDRV_SECTOR_SIZE - 1)) == 0) by
>>assert(QEMU_IS_ALIGNED(x, BDRV_SECTOR_SIZE)) in patch 3 (conflict with
>>“block: Use QEMU_IS_ALIGNED”), and
>> - Replaced the remaining instance of “qcow2_co_do_pwritev()” by
>>“qcow2_co_pwritev_task()” in a comment in patch 4
>>
>> and applied the series to my block branch:
>>
>> https://git.xanclic.moe/XanClic/qemu/commits/branch/block
> 
> Unfortunately, I’ll have to unstage the series for now because the fix
> to 026’s reference output isn’t stable.
> 
> When running the test in parallel (I can reproduce it with four
> instances on my machine with two cores + HT), I get failures like:
> 
> 026  fail   [15:21:09] [15:21:37]  (last: 18s)   output
> mismatch (see 026.out.bad)
> --- tests/qemu-iotests/026.out 2019-09-16 14:49:20.720410701 +0200
> +++ tests/qemu-iotests/026.out.bad   2019-09-16 15:21:37.180711936 +0200
> @@ -563,7 +563,7 @@
>   qemu-io: Failed to flush the refcount block cache: No space left on device
>   write failed: No space left on device
> 
> -74 leaked clusters were found on the image.
> +522 leaked clusters were found on the image.
>   This means waste of disk space, but no harm to data.
>   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
> 
> Failures: 026
> Failed 1 of 1 iotests
> 

Unfortunate enough:)

Hmm, can't reproduce, but I tend to fix this by just filtering out information 
about
leaked clusters in this test, as no sense in tracking it for failure paths, 
keeping
in mind newly introduced async handling of request parts.

-- 
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/4] iotests: add script_initialize

[Vladimir Sementsov-Ogievskiy]

12.09.2019 3:16, John Snow wrote:
> Like script_main, but doesn't require a single point of entry.
> Replace all existing initialization sections with this drop-in replacement.
> 
> This brings debug support to all existing script-style iotests.
> 
> Note: supported_oses=['linux'] was omitted, as it is a default argument.

But after this patch all test which didn't check os start to check linux
(as it's default).. So all tests which worked on other platforms will now
be skipped on these other platforms?

Finally do we support something except linux for iotests?
for bash tests _supported_os also used only with "Linux" in 87 tests..

May be we'd better drop both _supported_os and supported_oses alltogether,
and don't care?

Anyway, if we support only linux, any reason to skip almost all tests,
if someone tries to run test on other platform? Let him run what he wants.

> Signed-off-by: John Snow 
> ---

[..]

> +def execute_test(test_function=None, *args, **kwargs):
> +"""Run either unittest or script-style tests."""
> +
> +debug = execute_setup_common(*args, **kwargs)
>   if not test_function:
> -execute_unittest(output, verbosity, debug)
> +execute_unittest(debug)
>   else:
>   test_function()
>   
> +# This is called from script-style iotests without a single point of entry
> +def script_initialize(*args, **kwargs):
> +"""Initialize script-style tests without running any tests."""
> +execute_setup_common(*args, **kwargs)
> +
> +# This is called from script-style iotests with a single point of entry
>   def script_main(test_function, *args, **kwargs):
>   """Run script-style tests outside of the unittest framework"""
>   execute_test(test_function, *args, **kwargs)
>   
> +# This is called from unittest style iotests
>   def main(*args, **kwargs):
>   """Run tests using the unittest framework"""


Hmm, now two different styles of code documenting used: comment and doc-string,
both containing almost equal meaning.. I don't like it, still don't really mind.

>   execute_test(None, *args, **kwargs)
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PULL 00/16] Block patches

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190916142246.31474-1-mre...@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/20190916142246.31474-1-mre...@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [Qemu-block] [PATCH v7 0/3] Fix qcow2+luks corruption introduced by commit 8ac0f15f335

[Maxim Levitsky]

On Mon, 2019-09-16 at 16:59 +0300, Maxim Levitsky wrote:
> On Mon, 2019-09-16 at 15:39 +0200, Max Reitz wrote:
> > On 15.09.19 22:36, Maxim Levitsky wrote:
> > > Commit 8ac0f15f335 accidently broke the COW of non changed areas
> > > of newly allocated clusters, when the write spans multiple clusters,
> > > and needs COW both prior and after the write.
> > > This results in 'after' COW area being encrypted with wrong
> > > sector address, which render it corrupted.
> > > 
> > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922
> > > 
> > > CC: qemu-stable 
> > > 
> > > V2: grammar, spelling and code style fixes.
> > > V3: more fixes after the review.
> > > V4: addressed review comments from Max Reitz,
> > > and futher refactored the qcow2_co_encrypt to just take full host and 
> > > guest offset
> > > which simplifies everything.
> > > 
> > > V5: reworked the patches so one of them fixes the bug
> > > only and other one is just refactoring
> > > 
> > > V6: removed do_perform_cow_encrypt
> > > 
> > > V7: removed do_perform_cow_encrypt take two, this
> > > time I hopefully did that correctly :-)
> > > Also updated commit names and messages a bit
> > 
> > Luckily for you (maybe), Vladimir’s series doesn‘t quite pass the
> > iotests for me, so unfortunately (I find it unfortunate) I had to remove
> > it from my branch.  Thus, the conflicts are much more tame and I felt
> > comfortable taking the series to my branch (with the remaining trivial
> > conflicts resolved, and with Vladimir’s suggestion applied):
> > 
> > https://git.xanclic.moe/XanClic/qemu/commits/branch/block
> 
> 
> First of all, Thanks!
> 
> I don't know if this is luckily for me since I already rebased my series on 
> top of  
> https://git.xanclic.moe/XanClic/qemu.git,
> and run all qcow2 iotests, and only tests 
> 162 169 194 196 234 262 failed, and I know that 162 always fails
> due to that kernel change I talked about here few days ago,
> and rest for the AF_UNIX path len, which I need to do something
> about in the long term. I sometimes do a separate build in 
> directory which path doesn't trigger this, and sometimes,
> when I know that I haven't done significant changes to the patches,
> I just let these tests fail. In long term, maybe even in a few days
> I'll allocate some time to rethink the build environment here to
> fix that permanently.
> 
> Now I am rerunning the iotests just for fun, in short enough directory
> to see if I can reproduce the failure that you had. After looking
> in your report, that iotest 026 fails, it does pass here, but
> then I am only running these iotests on my laptop so I probably
> don't trigger the race you were able to.

Passed all the tests but 162, not that it matters much to be honest.
Best regards,
Maxim Levitsky

Re: [Qemu-block] [PATCH v6 28/42] stream: Deal with filters

[Kevin Wolf]

Am 16.09.2019 um 11:52 hat Max Reitz geschrieben:
> On 13.09.19 16:16, Kevin Wolf wrote:
> > Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
> >> @@ -261,16 +272,19 @@ void stream_start(const char *job_id, 
> >> BlockDriverState *bs,
> >>   * disappear from the chain after this operation. The streaming job 
> >> reads
> >>   * every block only once, assuming that it doesn't change, so forbid 
> >> writes
> >>   * and resizes. Reassign the base node pointer because the backing BS 
> >> of the
> >> - * bottom node might change after the call to 
> >> bdrv_reopen_set_read_only()
> >> - * due to parallel block jobs running.
> >> + * above_base node might change after the call to
> >> + * bdrv_reopen_set_read_only() due to parallel block jobs running.
> >>   */
> >> -base = backing_bs(bottom);
> >> -for (iter = backing_bs(bs); iter && iter != base; iter = 
> >> backing_bs(iter)) {
> >> +base = bdrv_filtered_bs(above_base);
> > 
> > We just calculated above_base such that it's the parent of base. Why
> > would base not already have the value we're assigning it again here?
> 
> That’s no change to existing code, whose reasoning is explained in the
> comment above: bdrv_reopen_set_read_only() can yield, which might lead
> to children of the bottom node changing.
> 
> If you feel like either that’s superfluous, or that if something like
> that were to happen we’d have much bigger problems, be my guest to drop
> both.
> 
> But in this series I’d rather just not change it.

Ah, you mean comments are there to be read?

But actually, I think iterating down to base is too much anyway. The
reasoning in the comment for block_job_add_bdrv() is that the nodes will
be dropped at the end. But base with all of its filter will be kept
after this patch.

So I think the for loop should stop after bs->base_overlay. And then
concurrently changing links aren't even a problem any more because
that's exactly the place up to which we've frozen the chain.

Kevin


signature.asc
Description: PGP signature

[Qemu-block] [PULL 15/16] block/qcow2: refactor encryption code

[Max Reitz]

From: Maxim Levitsky 

* Change the qcow2_co_{encrypt|decrypt} to just receive full host and
  guest offsets and use this function directly instead of calling
  do_perform_cow_encrypt (which is removed by that patch).

* Adjust qcow2_co_encdec to take full host and guest offsets as well.

* Document the qcow2_co_{encrypt|decrypt} arguments
  to prevent the bug fixed in former commit from hopefully
  happening again.

Signed-off-by: Maxim Levitsky 
Message-id: 20190915203655.21638-3-mlevi...@redhat.com
Reviewed-by: Vladimir Sementsov-Ogievskiy 
[mreitz: Let perform_cow() return the error value returned by
 qcow2_co_encrypt(), as proposed by Vladimir]
Signed-off-by: Max Reitz 
---
 block/qcow2.h |  8 +++---
 block/qcow2-cluster.c | 41 +---
 block/qcow2-threads.c | 63 +--
 block/qcow2.c |  5 ++--
 4 files changed, 69 insertions(+), 48 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 998bcdaef1..a488d761ff 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -758,10 +758,10 @@ ssize_t coroutine_fn
 qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
 const void *src, size_t src_size);
 int coroutine_fn
-qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
- uint64_t offset, void *buf, size_t len);
+qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_offset,
+ uint64_t guest_offset, void *buf, size_t len);
 int coroutine_fn
-qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
- uint64_t offset, void *buf, size_t len);
+qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
+ uint64_t guest_offset, void *buf, size_t len);
 
 #endif
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index cac0b6c7ba..8d5fa1539c 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -462,28 +462,6 @@ static int coroutine_fn 
do_perform_cow_read(BlockDriverState *bs,
 return 0;
 }
 
-static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
-uint64_t src_cluster_offset,
-uint64_t cluster_offset,
-unsigned offset_in_cluster,
-uint8_t *buffer,
-unsigned bytes)
-{
-if (bytes && bs->encrypted) {
-BDRVQcow2State *s = bs->opaque;
-assert(QEMU_IS_ALIGNED(offset_in_cluster, BDRV_SECTOR_SIZE));
-assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
-assert(s->crypto);
-if (qcow2_co_encrypt(bs,
-start_of_cluster(s, cluster_offset + offset_in_cluster),
-src_cluster_offset + offset_in_cluster,
-buffer, bytes) < 0) {
-return false;
-}
-}
-return true;
-}
-
 static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
  uint64_t cluster_offset,
  unsigned offset_in_cluster,
@@ -891,12 +869,19 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta 
*m)
 
 /* Encrypt the data if necessary before writing it */
 if (bs->encrypted) {
-if (!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
-start->offset, start_buffer,
-start->nb_bytes) ||
-!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
-end->offset, end_buffer, end->nb_bytes)) {
-ret = -EIO;
+ret = qcow2_co_encrypt(bs,
+   m->alloc_offset + start->offset,
+   m->offset + start->offset,
+   start_buffer, start->nb_bytes);
+if (ret < 0) {
+goto fail;
+}
+
+ret = qcow2_co_encrypt(bs,
+   m->alloc_offset + end->offset,
+   m->offset + end->offset,
+   end_buffer, end->nb_bytes);
+if (ret < 0) {
 goto fail;
 }
 }
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 3b1e63fe41..8f5a0d1ebe 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -234,35 +234,70 @@ static int qcow2_encdec_pool_func(void *opaque)
 }
 
 static int coroutine_fn
-qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
-  uint64_t offset, void *buf, size_t len, Qcow2EncDecFunc func)
+qcow2_co_encdec(BlockDriverState *bs, uint64_t host_offset,
+uint64_t guest_offset, void *buf, size_t len,
+Qcow2EncDecFunc func)
 {
 BDRVQcow2State *s = bs->opaque;
 Qcow2EncDecData arg = {
 .block = s->crypto,
-.offset = s->crypt

[Qemu-block] [PULL 12/16] curl: Check curl_multi_add_handle()'s return code

[Max Reitz]

If we had done that all along, debugging would have been much simpler.
(Also, I/O errors are better than hangs.)

Signed-off-by: Max Reitz 
Message-id: 20190910124136.10565-8-mre...@redhat.com
Reviewed-by: Maxim Levitsky 
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 block/curl.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/curl.c b/block/curl.c
index c343c7ed3d..f86299378e 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -882,7 +882,13 @@ static void curl_setup_preadv(BlockDriverState *bs, 
CURLAIOCB *acb)
 trace_curl_setup_preadv(acb->bytes, start, state->range);
 curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
 
-curl_multi_add_handle(s->multi, state->curl);
+if (curl_multi_add_handle(s->multi, state->curl) != CURLM_OK) {
+state->acb[0] = NULL;
+acb->ret = -EIO;
+
+curl_clean_state(state);
+goto out;
+}
 
 /* Tell curl it needs to kick things off */
 curl_multi_socket_action(s->multi, CURL_SOCKET_TIMEOUT, 0, &running);
-- 
2.21.0

[Qemu-block] [PULL 11/16] curl: Handle success in multi_check_completion

[Max Reitz]

Background: As of cURL 7.59.0, it verifies that several functions are
not called from within a callback.  Among these functions is
curl_multi_add_handle().

curl_read_cb() is a callback from cURL and not a coroutine.  Waking up
acb->co will lead to entering it then and there, which means the current
request will settle and the caller (if it runs in the same coroutine)
may then issue the next request.  In such a case, we will enter
curl_setup_preadv() effectively from within curl_read_cb().

Calling curl_multi_add_handle() will then fail and the new request will
not be processed.

Fix this by not letting curl_read_cb() wake up acb->co.  Instead, leave
the whole business of settling the AIOCB objects to
curl_multi_check_completion() (which is called from our timer callback
and our FD handler, so not from any cURL callbacks).

Reported-by: Natalie Gavrielov 
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1740193
Cc: qemu-sta...@nongnu.org
Signed-off-by: Max Reitz 
Message-id: 20190910124136.10565-7-mre...@redhat.com
Reviewed-by: John Snow 
Reviewed-by: Maxim Levitsky 
Signed-off-by: Max Reitz 
---
 block/curl.c | 69 ++--
 1 file changed, 29 insertions(+), 40 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index fd70f1ebc4..c343c7ed3d 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -229,7 +229,6 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t 
nmemb, void *opaque)
 {
 CURLState *s = ((CURLState*)opaque);
 size_t realsize = size * nmemb;
-int i;
 
 trace_curl_read_cb(realsize);
 
@@ -245,32 +244,6 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t 
nmemb, void *opaque)
 memcpy(s->orig_buf + s->buf_off, ptr, realsize);
 s->buf_off += realsize;
 
-for(i=0; iacb[i];
-
-if (!acb)
-continue;
-
-if ((s->buf_off >= acb->end)) {
-size_t request_length = acb->bytes;
-
-qemu_iovec_from_buf(acb->qiov, 0, s->orig_buf + acb->start,
-acb->end - acb->start);
-
-if (acb->end - acb->start < request_length) {
-size_t offset = acb->end - acb->start;
-qemu_iovec_memset(acb->qiov, offset, 0,
-  request_length - offset);
-}
-
-acb->ret = 0;
-s->acb[i] = NULL;
-qemu_mutex_unlock(&s->s->mutex);
-aio_co_wake(acb->co);
-qemu_mutex_lock(&s->s->mutex);
-}
-}
-
 read_end:
 /* curl will error out if we do not return this value */
 return size * nmemb;
@@ -351,13 +324,14 @@ static void curl_multi_check_completion(BDRVCURLState *s)
 break;
 
 if (msg->msg == CURLMSG_DONE) {
+int i;
 CURLState *state = NULL;
+bool error = msg->data.result != CURLE_OK;
+
 curl_easy_getinfo(msg->easy_handle, CURLINFO_PRIVATE,
   (char **)&state);
 
-/* ACBs for successful messages get completed in curl_read_cb */
-if (msg->data.result != CURLE_OK) {
-int i;
+if (error) {
 static int errcount = 100;
 
 /* Don't lose the original error message from curl, since
@@ -369,20 +343,35 @@ static void curl_multi_check_completion(BDRVCURLState *s)
 error_report("curl: further errors suppressed");
 }
 }
+}
 
-for (i = 0; i < CURL_NUM_ACB; i++) {
-CURLAIOCB *acb = state->acb[i];
+for (i = 0; i < CURL_NUM_ACB; i++) {
+CURLAIOCB *acb = state->acb[i];
 
-if (acb == NULL) {
-continue;
-}
+if (acb == NULL) {
+continue;
+}
+
+if (!error) {
+/* Assert that we have read all data */
+assert(state->buf_off >= acb->end);
+
+qemu_iovec_from_buf(acb->qiov, 0,
+state->orig_buf + acb->start,
+acb->end - acb->start);
 
-acb->ret = -EIO;
-state->acb[i] = NULL;
-qemu_mutex_unlock(&s->mutex);
-aio_co_wake(acb->co);
-qemu_mutex_lock(&s->mutex);
+if (acb->end - acb->start < acb->bytes) {
+size_t offset = acb->end - acb->start;
+qemu_iovec_memset(acb->qiov, offset, 0,
+  acb->bytes - offset);
+}
 }
+
+acb->ret = error ? -EIO : 0;
+state->acb[i] = NULL;
+qemu_mutex_unlock(&s->mutex);
+aio_co_wake(acb->co);
+qemu_mutex_lock(&s->mutex);
   

[Qemu-block] [PULL 10/16] curl: Report only ready sockets

[Max Reitz]

Instead of reporting all sockets to cURL, only report the one that has
caused curl_multi_do_locked() to be called.  This lets us get rid of the
QLIST_FOREACH_SAFE() list, which was actually wrong: SAFE foreaches are
only safe when the current element is removed in each iteration.  If it
possible for the list to be concurrently modified, we cannot guarantee
that only the current element will be removed.  Therefore, we must not
use QLIST_FOREACH_SAFE() here.

Fixes: ff5ca1664af85b24a4180d595ea6873fd3deac57
Cc: qemu-sta...@nongnu.org
Signed-off-by: Max Reitz 
Message-id: 20190910124136.10565-6-mre...@redhat.com
Reviewed-by: Maxim Levitsky 
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 block/curl.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index cf2686218d..fd70f1ebc4 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -392,24 +392,19 @@ static void curl_multi_check_completion(BDRVCURLState *s)
 }
 
 /* Called with s->mutex held.  */
-static void curl_multi_do_locked(CURLSocket *ready_socket)
+static void curl_multi_do_locked(CURLSocket *socket)
 {
-CURLSocket *socket, *next_socket;
-CURLState *s = ready_socket->state;
+BDRVCURLState *s = socket->state->s;
 int running;
 int r;
 
-if (!s->s->multi) {
+if (!s->multi) {
 return;
 }
 
-/* Need to use _SAFE because curl_multi_socket_action() may trigger
- * curl_sock_cb() which might modify this list */
-QLIST_FOREACH_SAFE(socket, &s->sockets, next, next_socket) {
-do {
-r = curl_multi_socket_action(s->s->multi, socket->fd, 0, &running);
-} while (r == CURLM_CALL_MULTI_PERFORM);
-}
+do {
+r = curl_multi_socket_action(s->multi, socket->fd, 0, &running);
+} while (r == CURLM_CALL_MULTI_PERFORM);
 }
 
 static void curl_multi_do(void *arg)
-- 
2.21.0

[Qemu-block] [PULL 07/16] curl: Keep *socket until the end of curl_sock_cb()

[Max Reitz]

This does not really change anything, but it makes the code a bit easier
to follow once we use @socket as the opaque pointer for
aio_set_fd_handler().

Cc: qemu-sta...@nongnu.org
Signed-off-by: Max Reitz 
Message-id: 20190910124136.10565-3-mre...@redhat.com
Reviewed-by: Maxim Levitsky 
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 block/curl.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 92dc2f630e..95d7b77dc0 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -172,10 +172,6 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int 
action,
 
 QLIST_FOREACH(socket, &state->sockets, next) {
 if (socket->fd == fd) {
-if (action == CURL_POLL_REMOVE) {
-QLIST_REMOVE(socket, next);
-g_free(socket);
-}
 break;
 }
 }
@@ -185,7 +181,6 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int 
action,
 socket->state = state;
 QLIST_INSERT_HEAD(&state->sockets, socket, next);
 }
-socket = NULL;
 
 trace_curl_sock_cb(action, (int)fd);
 switch (action) {
@@ -207,6 +202,11 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int 
action,
 break;
 }
 
+if (action == CURL_POLL_REMOVE) {
+QLIST_REMOVE(socket, next);
+g_free(socket);
+}
+
 return 0;
 }
 
-- 
2.21.0

[Qemu-block] [PULL 00/16] Block patches

[Max Reitz]

The following changes since commit dd25f97c66a75d1508f1d4c6478ed2c95bec428f:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20190913' 
into staging (2019-09-16 10:15:15 +0100)

are available in the Git repository at:

  https://github.com/XanClic/qemu.git tags/pull-block-2019-09-16

for you to fetch changes up to 1825cc0783ccf0ec5d9f0b225a99b340bdd4c68f:

  qemu-iotests: Add test for bz #1745922 (2019-09-16 15:37:12 +0200)


Block patches:
- Fix for block jobs when used with I/O threads
- Fix for a corruption when using qcow2's LUKS encryption mode
- cURL fix
- check-block.sh cleanups (for make check)
- Refactoring


Max Reitz (7):
  curl: Keep pointer to the CURLState in CURLSocket
  curl: Keep *socket until the end of curl_sock_cb()
  curl: Check completion in curl_multi_do()
  curl: Pass CURLSocket to curl_multi_do()
  curl: Report only ready sockets
  curl: Handle success in multi_check_completion
  curl: Check curl_multi_add_handle()'s return code

Maxim Levitsky (3):
  block/qcow2: Fix corruption introduced by commit 8ac0f15f335
  block/qcow2: refactor encryption code
  qemu-iotests: Add test for bz #1745922

Nir Soffer (2):
  block: Use QEMU_IS_ALIGNED
  block: Remove unused masks

Sergio Lopez (1):
  blockjob: update nodes head while removing all bdrv

Thomas Huth (2):
  tests/qemu-iotests/check: Replace "tests" with "iotests" in final
status text
  tests/Makefile: Do not print the name of the check-block.sh shell
script

Vladimir Sementsov-Ogievskiy (1):
  tests/qemu-iotests: Fix qemu-io related output in 026.out.nocache

 tests/Makefile.include |   2 +-
 block/qcow2.h  |   8 +-
 include/block/block.h  |   2 -
 block/bochs.c  |   4 +-
 block/cloop.c  |   4 +-
 block/curl.c   | 133 ++-
 block/dmg.c|   4 +-
 block/io.c |   8 +-
 block/qcow2-cluster.c  |  40 +++
 block/qcow2-threads.c  |  63 ---
 block/qcow2.c  |   9 +-
 block/vvfat.c  |   8 +-
 blockjob.c |  17 ++-
 migration/block.c  |   2 +-
 qemu-img.c |   2 +-
 tests/qemu-iotests/026.out.nocache | 168 ++---
 tests/qemu-iotests/263 |  91 
 tests/qemu-iotests/263.out |  40 +++
 tests/qemu-iotests/check   |   8 +-
 tests/qemu-iotests/group   |   1 +
 20 files changed, 380 insertions(+), 234 deletions(-)
 create mode 100755 tests/qemu-iotests/263
 create mode 100644 tests/qemu-iotests/263.out

-- 
2.21.0

[Qemu-block] [PATCH 4/4] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next

[Vladimir Sementsov-Ogievskiy]

bdrv_dirty_bitmap_next is always used in same pattern. So, split it
into _next and _first, instead of combining two functions into one and
add FOR_EACH_DIRTY_BITMAP macro.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/dirty-bitmap.h   |  9 +++--
 block.c|  4 +---
 block/dirty-bitmap.c   | 11 +++
 block/qcow2-bitmap.c   |  8 ++--
 migration/block-dirty-bitmap.c |  4 +---
 5 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 4c58d922e4..89e52db7ec 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -97,8 +97,13 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap 
*bitmap);
 bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
 bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
-BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
-BdrvDirtyBitmap *bitmap);
+
+BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs);
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap);
+#define FOR_EACH_DIRTY_BITMAP(bs, bitmap) \
+for (bitmap = bdrv_dirty_bitmap_first(bs); bitmap; \
+ bitmap = bdrv_dirty_bitmap_next(bitmap))
+
 char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
 int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset,
 uint64_t bytes);
diff --git a/block.c b/block.c
index 5944124845..96c2c5ae2d 100644
--- a/block.c
+++ b/block.c
@@ -5363,9 +5363,7 @@ static void coroutine_fn 
bdrv_co_invalidate_cache(BlockDriverState *bs,
 }
 }
 
-for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm;
- bm = bdrv_dirty_bitmap_next(bs, bm))
-{
+FOR_EACH_DIRTY_BITMAP(bs, bm) {
 bdrv_dirty_bitmap_skip_store(bm, false);
 }
 
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 76a8e59e61..e2df82af01 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -742,11 +742,14 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState 
*bs)
 return false;
 }
 
-BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
-BdrvDirtyBitmap *bitmap)
+BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs)
 {
-return bitmap == NULL ? QLIST_FIRST(&bs->dirty_bitmaps) :
-QLIST_NEXT(bitmap, list);
+return QLIST_FIRST(&bs->dirty_bitmaps);
+}
+
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap)
+{
+return QLIST_NEXT(bitmap, list);
 }
 
 char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 6d795a2255..73ebd2ff6a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1480,9 +1480,7 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 }
 
 /* check constraints and names */
-for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
- bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
-{
+FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
 const char *name = bdrv_dirty_bitmap_name(bitmap);
 uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
 Qcow2Bitmap *bm;
@@ -1602,9 +1600,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error 
**errp)
 return -EINVAL;
 }
 
-for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
- bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
-{
+FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
 if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
 bdrv_dirty_bitmap_set_readonly(bitmap, true);
 }
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 793f249aa5..7eafface61 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -283,9 +283,7 @@ static int init_dirty_bitmap_migration(void)
 for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) {
 const char *name = bdrv_get_device_or_node_name(bs);
 
-for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
- bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
-{
+FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
 if (!bdrv_dirty_bitmap_name(bitmap)) {
 continue;
 }
-- 
2.21.0

[Qemu-block] [PULL 16/16] qemu-iotests: Add test for bz #1745922

[Max Reitz]

From: Maxim Levitsky 

Signed-off-by: Maxim Levitsky 
Tested-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20190915203655.21638-4-mlevi...@redhat.com
Reviewed-by: Max Reitz 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/263 | 91 ++
 tests/qemu-iotests/263.out | 40 +
 tests/qemu-iotests/group   |  1 +
 3 files changed, 132 insertions(+)
 create mode 100755 tests/qemu-iotests/263
 create mode 100644 tests/qemu-iotests/263.out

diff --git a/tests/qemu-iotests/263 b/tests/qemu-iotests/263
new file mode 100755
index 00..d2c030fae9
--- /dev/null
+++ b/tests/qemu-iotests/263
@@ -0,0 +1,91 @@
+#!/usr/bin/env bash
+#
+# Test encrypted write that crosses cluster boundary of two unallocated 
clusters
+# Based on 188
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# 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=mlevi...@redhat.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 generic
+_supported_os Linux
+
+
+size=1M
+
+SECRET="secret,id=sec0,data=astrochicken"
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
+
+
+_run_test()
+{
+   echo "== reading the whole image =="
+   $QEMU_IO --object $SECRET -c "read -P 0 0 $size" --image-opts "$1" | 
_filter_qemu_io | _filter_testdir
+
+   echo
+   echo "== write two 512 byte sectors on a cluster boundary =="
+   $QEMU_IO --object $SECRET -c "write -P 0xAA 0xFE00 0x400" --image-opts 
"$1" | _filter_qemu_io | _filter_testdir
+
+   echo
+   echo "== verify that the rest of the image is not changed =="
+   $QEMU_IO --object $SECRET -c "read -P 0x00 0x0 0xFE00" --image-opts 
"$1" | _filter_qemu_io | _filter_testdir
+   $QEMU_IO --object $SECRET -c "read -P 0xAA 0x0FE00 0x400" --image-opts 
"$1" | _filter_qemu_io | _filter_testdir
+   $QEMU_IO --object $SECRET -c "read -P 0x00 0x10200 0xEFE00" 
--image-opts "$1" | _filter_qemu_io | _filter_testdir
+
+}
+
+
+echo
+echo "testing LUKS qcow2 encryption"
+echo
+
+_make_test_img --object $SECRET -o 
"encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10,cluster_size=64K"
 $size
+_run_test "driver=$IMGFMT,encrypt.key-secret=sec0,file.filename=$TEST_IMG"
+_cleanup_test_img
+
+echo
+echo "testing legacy AES qcow2 encryption"
+echo
+
+
+_make_test_img --object $SECRET -o 
"encrypt.format=aes,encrypt.key-secret=sec0,cluster_size=64K" $size
+_run_test "driver=$IMGFMT,encrypt.key-secret=sec0,file.filename=$TEST_IMG"
+_cleanup_test_img
+
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/263.out b/tests/qemu-iotests/263.out
new file mode 100644
index 00..0c982c55cb
--- /dev/null
+++ b/tests/qemu-iotests/263.out
@@ -0,0 +1,40 @@
+QA output created by 263
+
+testing LUKS qcow2 encryption
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks 
encrypt.key-secret=sec0 encrypt.iter-time=10
+== reading the whole image ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== write two 512 byte sectors on a cluster boundary ==
+wrote 1024/1024 bytes at offset 65024
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verify that the rest of the image is not changed ==
+read 65024/65024 bytes at offset 0
+63.500 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1024/1024 bytes at offset 65024
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 982528/982528 bytes at offset 66048
+959.500 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+testing legacy AES qcow2 encryption
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=aes 
encrypt.key-secret=sec0
+== reading the whole image ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== write two 512 byte sectors on a cluster boundary ==
+wrote 1024/1024 bytes at offset 65024
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verify that the rest of the image is not changed ==
+read 65024/65024 bytes at offset 0
+63.500 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1024/1024

[Qemu-block] [PULL 01/16] block: Use QEMU_IS_ALIGNED

[Max Reitz]

From: Nir Soffer 

Replace instances of:

(n & (BDRV_SECTOR_SIZE - 1)) == 0

And:

   (n & ~BDRV_SECTOR_MASK) == 0

With:

QEMU_IS_ALIGNED(n, BDRV_SECTOR_SIZE)

Which reveals the intent of the code better, and makes it easier to
locate the code checking alignment.

Signed-off-by: Nir Soffer 
Message-id: 20190827185913.27427-2-nsof...@redhat.com
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 block/bochs.c | 4 ++--
 block/cloop.c | 4 ++--
 block/dmg.c   | 4 ++--
 block/io.c| 8 
 block/qcow2-cluster.c | 4 ++--
 block/qcow2.c | 4 ++--
 block/vvfat.c | 8 
 qemu-img.c| 2 +-
 8 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/block/bochs.c b/block/bochs.c
index 962f18592d..32bb83b268 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -248,8 +248,8 @@ bochs_co_preadv(BlockDriverState *bs, uint64_t offset, 
uint64_t bytes,
 QEMUIOVector local_qiov;
 int ret;
 
-assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
-assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
+assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
 
 qemu_iovec_init(&local_qiov, qiov->niov);
 qemu_co_mutex_lock(&s->lock);
diff --git a/block/cloop.c b/block/cloop.c
index 384c9735bb..4de94876d4 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -253,8 +253,8 @@ cloop_co_preadv(BlockDriverState *bs, uint64_t offset, 
uint64_t bytes,
 int nb_sectors = bytes >> BDRV_SECTOR_BITS;
 int ret, i;
 
-assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
-assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
+assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
 
 qemu_co_mutex_lock(&s->lock);
 
diff --git a/block/dmg.c b/block/dmg.c
index 45f6b28f17..4a045f2b3e 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -697,8 +697,8 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, 
uint64_t bytes,
 int nb_sectors = bytes >> BDRV_SECTOR_BITS;
 int ret, i;
 
-assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
-assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
+assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
 
 qemu_co_mutex_lock(&s->lock);
 
diff --git a/block/io.c b/block/io.c
index 16a598fd08..f8c3596131 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1097,8 +1097,8 @@ static int coroutine_fn 
bdrv_driver_preadv(BlockDriverState *bs,
 sector_num = offset >> BDRV_SECTOR_BITS;
 nb_sectors = bytes >> BDRV_SECTOR_BITS;
 
-assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
-assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
+assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
 assert(bytes <= BDRV_REQUEST_MAX_BYTES);
 assert(drv->bdrv_co_readv);
 
@@ -1171,8 +1171,8 @@ static int coroutine_fn 
bdrv_driver_pwritev(BlockDriverState *bs,
 sector_num = offset >> BDRV_SECTOR_BITS;
 nb_sectors = bytes >> BDRV_SECTOR_BITS;
 
-assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
-assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
+assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
 assert(bytes <= BDRV_REQUEST_MAX_BYTES);
 
 assert(drv->bdrv_co_writev);
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index dcacd3c450..cb44b6c6ba 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -471,8 +471,8 @@ static bool coroutine_fn 
do_perform_cow_encrypt(BlockDriverState *bs,
 {
 if (bytes && bs->encrypted) {
 BDRVQcow2State *s = bs->opaque;
-assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
-assert((bytes & ~BDRV_SECTOR_MASK) == 0);
+assert(QEMU_IS_ALIGNED(offset_in_cluster, BDRV_SECTOR_SIZE));
+assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
 assert(s->crypto);
 if (qcow2_co_encrypt(bs, cluster_offset,
  src_cluster_offset + offset_in_cluster,
diff --git a/block/qcow2.c b/block/qcow2.c
index 57734f20cf..cac18f0ba2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2067,8 +2067,8 @@ static coroutine_fn int 
qcow2_co_preadv_part(BlockDriverState *bs,
 goto fail;
 }
 
-assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
-assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
+assert(QEMU_IS_ALIGNED(cur_bytes, BDRV_SECTOR_SIZE));
 if (qcow2_co_decrypt(bs, cluster_offset, offset,
  cluster_data, cur_bytes) < 0) {
 ret = -EIO;
diff --git a/block/vvfat.c b/block/vvfat.c
index f6c28805dd..019b8f1341 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1547,8 +1547,8 @@ vvfat_co_preadv(BlockDriverState *bs, 

[Qemu-block] [PULL 08/16] curl: Check completion in curl_multi_do()

[Max Reitz]

While it is more likely that transfers complete after some file
descriptor has data ready to read, we probably should not rely on it.
Better be safe than sorry and call curl_multi_check_completion() in
curl_multi_do(), too, just like it is done in curl_multi_read().

With this change, curl_multi_do() and curl_multi_read() are actually the
same, so drop curl_multi_read() and use curl_multi_do() as the sole FD
handler.

Signed-off-by: Max Reitz 
Message-id: 20190910124136.10565-4-mre...@redhat.com
Reviewed-by: Maxim Levitsky 
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 block/curl.c | 14 ++
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 95d7b77dc0..5838afef99 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -139,7 +139,6 @@ typedef struct BDRVCURLState {
 
 static void curl_clean_state(CURLState *s);
 static void curl_multi_do(void *arg);
-static void curl_multi_read(void *arg);
 
 #ifdef NEED_CURL_TIMER_CALLBACK
 /* Called from curl_multi_do_locked, with s->mutex held.  */
@@ -186,7 +185,7 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int 
action,
 switch (action) {
 case CURL_POLL_IN:
 aio_set_fd_handler(s->aio_context, fd, false,
-   curl_multi_read, NULL, NULL, state);
+   curl_multi_do, NULL, NULL, state);
 break;
 case CURL_POLL_OUT:
 aio_set_fd_handler(s->aio_context, fd, false,
@@ -194,7 +193,7 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int 
action,
 break;
 case CURL_POLL_INOUT:
 aio_set_fd_handler(s->aio_context, fd, false,
-   curl_multi_read, curl_multi_do, NULL, state);
+   curl_multi_do, curl_multi_do, NULL, state);
 break;
 case CURL_POLL_REMOVE:
 aio_set_fd_handler(s->aio_context, fd, false,
@@ -416,15 +415,6 @@ static void curl_multi_do(void *arg)
 {
 CURLState *s = (CURLState *)arg;
 
-qemu_mutex_lock(&s->s->mutex);
-curl_multi_do_locked(s);
-qemu_mutex_unlock(&s->s->mutex);
-}
-
-static void curl_multi_read(void *arg)
-{
-CURLState *s = (CURLState *)arg;
-
 qemu_mutex_lock(&s->s->mutex);
 curl_multi_do_locked(s);
 curl_multi_check_completion(s->s);
-- 
2.21.0

[Qemu-block] [PULL 05/16] tests/qemu-iotests: Fix qemu-io related output in 026.out.nocache

[Max Reitz]

From: Vladimir Sementsov-Ogievskiy 

qemu-io now prefixes its error and warnings with "qemu-io:".
36b9986b08787019e fixed a lot of iotests output but forget about
026.out.nocache. Fix it too.

Fixes: 99e98d7c9fc1a1639fad ("qemu-io: Use error_[gs]et_progname()")
Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20190816153015.447957-2-vsement...@virtuozzo.com
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/026.out.nocache | 168 ++---
 1 file changed, 84 insertions(+), 84 deletions(-)

diff --git a/tests/qemu-iotests/026.out.nocache 
b/tests/qemu-iotests/026.out.nocache
index 1ca6cda15c..6dda95dfb4 100644
--- a/tests/qemu-iotests/026.out.nocache
+++ b/tests/qemu-iotests/026.out.nocache
@@ -14,8 +14,8 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824 
 
 Event: l1_update; errno: 5; imm: off; once: off; write 
-Failed to flush the L2 table cache: Input/output error
-Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to flush the L2 table cache: Input/output error
+qemu-io: Failed to flush the refcount block cache: Input/output error
 write failed: Input/output error
 
 1 leaked clusters were found on the image.
@@ -23,8 +23,8 @@ This means waste of disk space, but no harm to data.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824 
 
 Event: l1_update; errno: 5; imm: off; once: off; write -b
-Failed to flush the L2 table cache: Input/output error
-Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to flush the L2 table cache: Input/output error
+qemu-io: Failed to flush the refcount block cache: Input/output error
 write failed: Input/output error
 
 1 leaked clusters were found on the image.
@@ -42,8 +42,8 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824 
 
 Event: l1_update; errno: 28; imm: off; once: off; write 
-Failed to flush the L2 table cache: No space left on device
-Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to flush the L2 table cache: No space left on device
+qemu-io: Failed to flush the refcount block cache: No space left on device
 write failed: No space left on device
 
 1 leaked clusters were found on the image.
@@ -51,8 +51,8 @@ This means waste of disk space, but no harm to data.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824 
 
 Event: l1_update; errno: 28; imm: off; once: off; write -b
-Failed to flush the L2 table cache: No space left on device
-Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to flush the L2 table cache: No space left on device
+qemu-io: Failed to flush the refcount block cache: No space left on device
 write failed: No space left on device
 
 1 leaked clusters were found on the image.
@@ -136,8 +136,8 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_update; errno: 5; imm: off; once: off; write 
-Failed to flush the L2 table cache: Input/output error
-Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to flush the L2 table cache: Input/output error
+qemu-io: Failed to flush the refcount block cache: Input/output error
 wrote 131072/131072 bytes at offset 0
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -146,8 +146,8 @@ This means waste of disk space, but no harm to data.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824 
 
 Event: l2_update; errno: 5; imm: off; once: off; write -b
-Failed to flush the L2 table cache: Input/output error
-Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to flush the L2 table cache: Input/output error
+qemu-io: Failed to flush the refcount block cache: Input/output error
 wrote 131072/131072 bytes at offset 0
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -168,8 +168,8 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_update; errno: 28; imm: off; once: off; write 
-Failed to flush the L2 table cache: No space left on device
-Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to flush the L2 table cache: No space left on device
+qemu-io: Failed to flush the refcount block cache: No space left on device
 wrote 131072/131072 bytes at offset 0
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -178,8 +178,8 @@ This means waste of disk space, but no harm to data.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824 
 
 Event: l2_update; errno: 28; imm: off; once: off; write -b
-Failed to flush the L2 table cache: No space left on device
-Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to flush the L2 table cache: No space left on device
+qemu-io: Failed to flush the refcount block cache: No space left on device
 wrote 131072/131072 bytes at offset

[Qemu-block] [PULL 04/16] tests/Makefile: Do not print the name of the check-block.sh shell script

[Max Reitz]

From: Thomas Huth 

The check script is already printing out which iotest is currently
running, so printing out the name of the check-block.sh shell script
looks superfluous here.

Signed-off-by: Thomas Huth 
Message-id: 20190906113534.10907-1-th...@redhat.com
Acked-by: John Snow 
Reviewed-by: Max Reitz 
Signed-off-by: Max Reitz 
---
 tests/Makefile.include | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index b39860a8d0..793632ca72 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -1100,7 +1100,7 @@ QEMU_IOTESTS_HELPERS-$(call 
land,$(CONFIG_SOFTMMU),$(CONFIG_LINUX)) = tests/qemu
 check-tests/check-block.sh: tests/check-block.sh qemu-img$(EXESUF) \
qemu-io$(EXESUF) qemu-nbd$(EXESUF) $(QEMU_IOTESTS_HELPERS-y) \
$(patsubst %,%/all,$(filter %-softmmu,$(TARGET_DIRS)))
-   $<
+   @$<
 
 .PHONY: $(patsubst %, check-%, $(check-qapi-schema-y))
 $(patsubst %, check-%, $(check-qapi-schema-y)): check-%.json: 
$(SRC_PATH)/%.json
-- 
2.21.0

[Qemu-block] [PULL 14/16] block/qcow2: Fix corruption introduced by commit 8ac0f15f335

[Max Reitz]

From: Maxim Levitsky 

This fixes subtle corruption introduced by luks threaded encryption
in commit 8ac0f15f335

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922

The corruption happens when we do a write that
   * writes to two or more unallocated clusters at once
   * doesn't fully cover the first sector
   * doesn't fully cover the last sector
   * uses luks encryption

In this case, when allocating the new clusters we COW both areas
prior to the write and after the write, and we encrypt them.

The above mentioned commit accidentally made it so we encrypt the
second COW area using the physical cluster offset of the first area.

The problem is that offset_in_cluster in do_perform_cow_encrypt
can be larger that the cluster size, thus cluster_offset
will no longer point to the start of the cluster at which encrypted
area starts.

Next patch in this series will refactor the code to avoid all these
assumptions.

In the bugreport that was triggered by rebasing a luks image to new,
zero filled base, which lot of such writes, and causes some files
with zero areas to contain garbage there instead.
But as described above it can happen elsewhere as well

Signed-off-by: Maxim Levitsky 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20190915203655.21638-2-mlevi...@redhat.com
Reviewed-by: Max Reitz 
Signed-off-by: Max Reitz 
---
 block/qcow2-cluster.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index cb44b6c6ba..cac0b6c7ba 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -474,9 +474,10 @@ static bool coroutine_fn 
do_perform_cow_encrypt(BlockDriverState *bs,
 assert(QEMU_IS_ALIGNED(offset_in_cluster, BDRV_SECTOR_SIZE));
 assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
 assert(s->crypto);
-if (qcow2_co_encrypt(bs, cluster_offset,
- src_cluster_offset + offset_in_cluster,
- buffer, bytes) < 0) {
+if (qcow2_co_encrypt(bs,
+start_of_cluster(s, cluster_offset + offset_in_cluster),
+src_cluster_offset + offset_in_cluster,
+buffer, bytes) < 0) {
 return false;
 }
 }
-- 
2.21.0

[Qemu-block] [PULL 13/16] blockjob: update nodes head while removing all bdrv

[Max Reitz]

From: Sergio Lopez 

block_job_remove_all_bdrv() iterates through job->nodes, calling
bdrv_root_unref_child() for each entry. The call to the latter may
reach child_job_[can_]set_aio_ctx(), which will also attempt to
traverse job->nodes, potentially finding entries that where freed
on previous iterations.

To avoid this situation, update job->nodes head on each iteration to
ensure that already freed entries are no longer linked to the list.

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1746631
Signed-off-by: Sergio Lopez 
Cc: qemu-sta...@nongnu.org
Signed-off-by: Max Reitz 
Message-id: 20190911100316.32282-1-mre...@redhat.com
Reviewed-by: Sergio Lopez 
Signed-off-by: Max Reitz 
---
 blockjob.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index 2abed0f551..c6e20e2fcd 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -175,14 +175,23 @@ static const BdrvChildRole child_job = {
 
 void block_job_remove_all_bdrv(BlockJob *job)
 {
-GSList *l;
-for (l = job->nodes; l; l = l->next) {
+/*
+ * bdrv_root_unref_child() may reach child_job_[can_]set_aio_ctx(),
+ * which will also traverse job->nodes, so consume the list one by
+ * one to make sure that such a concurrent access does not attempt
+ * to process an already freed BdrvChild.
+ */
+while (job->nodes) {
+GSList *l = job->nodes;
 BdrvChild *c = l->data;
+
+job->nodes = l->next;
+
 bdrv_op_unblock_all(c->bs, job->blocker);
 bdrv_root_unref_child(c);
+
+g_slist_free_1(l);
 }
-g_slist_free(job->nodes);
-job->nodes = NULL;
 }
 
 bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs)
-- 
2.21.0

[Qemu-block] [PULL 03/16] tests/qemu-iotests/check: Replace "tests" with "iotests" in final status text

[Max Reitz]

From: Thomas Huth 

When running "make check -j8" or something similar, the iotests are
running in parallel with the other tests. So when they are printing
out "Passed all xx tests" or a similar status message at the end,
it might not be quite clear that this message belongs to the iotests,
since the output might be mixed with the other tests. Thus change the
word "tests" here to "iotests" instead to avoid confusion.

Signed-off-by: Thomas Huth 
Message-id: 20190906113920.11271-1-th...@redhat.com
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/check | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index a58232eefb..875399d79f 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -694,12 +694,12 @@ END{ if (NR > 0) {
 if [ ! -z "$n_bad" -a $n_bad != 0 ]
 then
 echo "Failures:$bad"
-echo "Failed $n_bad of $try tests"
+echo "Failed $n_bad of $try iotests"
 echo "Failures:$bad" | fmt >>check.log
-echo "Failed $n_bad of $try tests" >>check.log
+echo "Failed $n_bad of $try iotests" >>check.log
 else
-echo "Passed all $try tests"
-echo "Passed all $try tests" >>check.log
+echo "Passed all $try iotests"
+echo "Passed all $try iotests" >>check.log
 fi
 needwrap=false
 fi
-- 
2.21.0

[Qemu-block] [PULL 09/16] curl: Pass CURLSocket to curl_multi_do()

[Max Reitz]

curl_multi_do_locked() currently marks all sockets as ready.  That is
not only inefficient, but in fact unsafe (the loop is).  A follow-up
patch will change that, but to do so, curl_multi_do_locked() needs to
know exactly which socket is ready; and that is accomplished by this
patch here.

Cc: qemu-sta...@nongnu.org
Signed-off-by: Max Reitz 
Message-id: 20190910124136.10565-5-mre...@redhat.com
Reviewed-by: Maxim Levitsky 
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 block/curl.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 5838afef99..cf2686218d 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -185,15 +185,15 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int 
action,
 switch (action) {
 case CURL_POLL_IN:
 aio_set_fd_handler(s->aio_context, fd, false,
-   curl_multi_do, NULL, NULL, state);
+   curl_multi_do, NULL, NULL, socket);
 break;
 case CURL_POLL_OUT:
 aio_set_fd_handler(s->aio_context, fd, false,
-   NULL, curl_multi_do, NULL, state);
+   NULL, curl_multi_do, NULL, socket);
 break;
 case CURL_POLL_INOUT:
 aio_set_fd_handler(s->aio_context, fd, false,
-   curl_multi_do, curl_multi_do, NULL, state);
+   curl_multi_do, curl_multi_do, NULL, socket);
 break;
 case CURL_POLL_REMOVE:
 aio_set_fd_handler(s->aio_context, fd, false,
@@ -392,9 +392,10 @@ static void curl_multi_check_completion(BDRVCURLState *s)
 }
 
 /* Called with s->mutex held.  */
-static void curl_multi_do_locked(CURLState *s)
+static void curl_multi_do_locked(CURLSocket *ready_socket)
 {
 CURLSocket *socket, *next_socket;
+CURLState *s = ready_socket->state;
 int running;
 int r;
 
@@ -413,12 +414,13 @@ static void curl_multi_do_locked(CURLState *s)
 
 static void curl_multi_do(void *arg)
 {
-CURLState *s = (CURLState *)arg;
+CURLSocket *socket = arg;
+BDRVCURLState *s = socket->state->s;
 
-qemu_mutex_lock(&s->s->mutex);
-curl_multi_do_locked(s);
-curl_multi_check_completion(s->s);
-qemu_mutex_unlock(&s->s->mutex);
+qemu_mutex_lock(&s->mutex);
+curl_multi_do_locked(socket);
+curl_multi_check_completion(s);
+qemu_mutex_unlock(&s->mutex);
 }
 
 static void curl_multi_timeout_do(void *arg)
-- 
2.21.0

[Qemu-block] [PATCH 2/4] block/dirty-bitmap: add bs link

[Vladimir Sementsov-Ogievskiy]

Add bs field to BdrvDirtyBitmap structure. Drop BlockDriverState
parameter from bitmap APIs where possible.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/dirty-bitmap.h   | 14 +-
 block/backup.c | 14 ++
 block/dirty-bitmap.c   | 24 
 block/mirror.c |  4 ++--
 block/qcow2-bitmap.c   |  6 +++---
 blockdev.c |  6 +++---
 migration/block-dirty-bitmap.c |  7 +++
 migration/block.c  |  4 ++--
 8 files changed, 36 insertions(+), 43 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 848dfc6590..4c58d922e4 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -18,21 +18,18 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
*bs,
   uint32_t granularity,
   const char *name,
   Error **errp);
-int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
-   BdrvDirtyBitmap *bitmap,
+int bdrv_dirty_bitmap_create_successor(BdrvDirtyBitmap *bitmap,
Error **errp);
-BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
-BdrvDirtyBitmap *bitmap,
+BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BdrvDirtyBitmap *bitmap,
 Error **errp);
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
-   BdrvDirtyBitmap *bitmap,
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BdrvDirtyBitmap *bitmap,
Error **errp);
 void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap);
 BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
 const char *name);
 int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
 Error **errp);
-void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_release_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
 void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
  const char *name,
@@ -107,8 +104,7 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap 
*bitmap, uint64_t offset,
 uint64_t bytes);
 bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
uint64_t *offset, uint64_t *bytes);
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
-  BdrvDirtyBitmap *bitmap,
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
   Error **errp);
 
 #endif
diff --git a/block/backup.c b/block/backup.c
index 763f0d7ff6..acb67da3a7 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -352,7 +352,6 @@ static int coroutine_fn backup_before_write_notify(
 static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
 {
 BdrvDirtyBitmap *bm;
-BlockDriverState *bs = blk_bs(job->common.blk);
 bool sync = (((ret == 0) || (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS)) 
\
  && (job->bitmap_mode != BITMAP_SYNC_MODE_NEVER));
 
@@ -361,13 +360,13 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob 
*job, int ret)
  * We succeeded, or we always intended to sync the bitmap.
  * Delete this bitmap and install the child.
  */
-bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL);
+bm = bdrv_dirty_bitmap_abdicate(job->sync_bitmap, NULL);
 } else {
 /*
  * We failed, or we never intended to sync the bitmap anyway.
  * Merge the successor back into the parent, keeping all data.
  */
-bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
+bm = bdrv_reclaim_dirty_bitmap(job->sync_bitmap, NULL);
 }
 
 assert(bm);
@@ -398,10 +397,9 @@ static void backup_abort(Job *job)
 static void backup_clean(Job *job)
 {
 BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
-BlockDriverState *bs = blk_bs(s->common.blk);
 
 if (s->copy_bitmap) {
-bdrv_release_dirty_bitmap(bs, s->copy_bitmap);
+bdrv_release_dirty_bitmap(s->copy_bitmap);
 s->copy_bitmap = NULL;
 }
 
@@ -679,7 +677,7 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 }
 
 /* Create a new bitmap, and freeze/disable this one. */
-if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) {
+if (bdrv_dirty_bitmap_create_successor(sync_bitmap, errp) < 0) {
 return NULL;
   

[Qemu-block] [PATCH 1/4] block/dirty-bitmap: drop meta

[Vladimir Sementsov-Ogievskiy]

Drop meta bitmaps, as they are unused.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/dirty-bitmap.h |  5 
 block/dirty-bitmap.c | 46 
 2 files changed, 51 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 4b4b731b46..848dfc6590 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -18,9 +18,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
*bs,
   uint32_t granularity,
   const char *name,
   Error **errp);
-void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
-   int chunk_size);
-void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
Error **errp);
@@ -55,7 +52,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t offset, int64_t bytes);
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
  int64_t offset, int64_t bytes);
-BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
 BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
 
@@ -97,7 +93,6 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
 int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
 void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
 int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
-int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);
 bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
 bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 134e0c9a0c..acfc3077f1 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -30,7 +30,6 @@
 struct BdrvDirtyBitmap {
 QemuMutex *mutex;
 HBitmap *bitmap;/* Dirty bitmap implementation */
-HBitmap *meta;  /* Meta dirty bitmap */
 bool busy;  /* Bitmap is busy, it can't be used via QMP */
 BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
 char *name; /* Optional non-empty unique ID */
@@ -126,36 +125,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
*bs,
 return bitmap;
 }
 
-/* bdrv_create_meta_dirty_bitmap
- *
- * Create a meta dirty bitmap that tracks the changes of bits in @bitmap. I.e.
- * when a dirty status bit in @bitmap is changed (either from reset to set or
- * the other way around), its respective meta dirty bitmap bit will be marked
- * dirty as well.
- *
- * @bitmap: the block dirty bitmap for which to create a meta dirty bitmap.
- * @chunk_size: how many bytes of bitmap data does each bit in the meta bitmap
- * track.
- */
-void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
-   int chunk_size)
-{
-assert(!bitmap->meta);
-qemu_mutex_lock(bitmap->mutex);
-bitmap->meta = hbitmap_create_meta(bitmap->bitmap,
-   chunk_size * BITS_PER_BYTE);
-qemu_mutex_unlock(bitmap->mutex);
-}
-
-void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
-{
-assert(bitmap->meta);
-qemu_mutex_lock(bitmap->mutex);
-hbitmap_free_meta(bitmap->bitmap);
-bitmap->meta = NULL;
-qemu_mutex_unlock(bitmap->mutex);
-}
-
 int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap)
 {
 return bitmap->size;
@@ -319,7 +288,6 @@ static void 
bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
 assert(!bitmap->active_iterators);
 assert(!bdrv_dirty_bitmap_busy(bitmap));
 assert(!bdrv_dirty_bitmap_has_successor(bitmap));
-assert(!bitmap->meta);
 QLIST_REMOVE(bitmap, list);
 hbitmap_free(bitmap->bitmap);
 g_free(bitmap->name);
@@ -557,15 +525,6 @@ BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap 
*bitmap)
 return iter;
 }
 
-BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap)
-{
-BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
-hbitmap_iter_init(&iter->hbi, bitmap->meta, 0);
-iter->bitmap = bitmap;
-bitmap->active_iterators++;
-return iter;
-}
-
 void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)
 {
 if (!iter) {
@@ -712,11 +671,6 @@ int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
 return hbitmap_count(bitmap->bitmap);
 }
 
-int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
-{
-return hbitmap_count(bitmap->meta);
-}
-
 bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap)
 {
 return bitmap->readonly;
-- 
2.21.0

[Qemu-block] [PULL 06/16] curl: Keep pointer to the CURLState in CURLSocket

[Max Reitz]

A follow-up patch will make curl_multi_do() and curl_multi_read() take a
CURLSocket instead of the CURLState.  They still need the latter,
though, so add a pointer to it to the former.

Cc: qemu-sta...@nongnu.org
Signed-off-by: Max Reitz 
Reviewed-by: John Snow 
Message-id: 20190910124136.10565-2-mre...@redhat.com
Reviewed-by: Maxim Levitsky 
Signed-off-by: Max Reitz 
---
 block/curl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/curl.c b/block/curl.c
index d4c8e94f3e..92dc2f630e 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -80,6 +80,7 @@ static CURLMcode __curl_multi_socket_action(CURLM 
*multi_handle,
 #define CURL_BLOCK_OPT_TIMEOUT_DEFAULT 5
 
 struct BDRVCURLState;
+struct CURLState;
 
 static bool libcurl_initialized;
 
@@ -97,6 +98,7 @@ typedef struct CURLAIOCB {
 
 typedef struct CURLSocket {
 int fd;
+struct CURLState *state;
 QLIST_ENTRY(CURLSocket) next;
 } CURLSocket;
 
@@ -180,6 +182,7 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int 
action,
 if (!socket) {
 socket = g_new0(CURLSocket, 1);
 socket->fd = fd;
+socket->state = state;
 QLIST_INSERT_HEAD(&state->sockets, socket, next);
 }
 socket = NULL;
-- 
2.21.0

[Qemu-block] [PULL 02/16] block: Remove unused masks

[Max Reitz]

From: Nir Soffer 

Replace confusing usage:

~BDRV_SECTOR_MASK

With more clear:

(BDRV_SECTOR_SIZE - 1)

Remove BDRV_SECTOR_MASK and the unused BDRV_BLOCK_OFFSET_MASK which was
it's last user.

Signed-off-by: Nir Soffer 
Message-id: 20190827185913.27427-3-nsof...@redhat.com
Reviewed-by: Juan Quintela 
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 include/block/block.h | 2 --
 migration/block.c | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 124ad40809..37c9de7446 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -143,7 +143,6 @@ typedef struct HDGeometry {
 
 #define BDRV_SECTOR_BITS   9
 #define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
-#define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
 
 #define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
  INT_MAX >> BDRV_SECTOR_BITS)
@@ -195,7 +194,6 @@ typedef struct HDGeometry {
 #define BDRV_BLOCK_ALLOCATED0x10
 #define BDRV_BLOCK_EOF  0x20
 #define BDRV_BLOCK_RECURSE  0x40
-#define BDRV_BLOCK_OFFSET_MASK  BDRV_SECTOR_MASK
 
 typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) 
BlockReopenQueue;
 
diff --git a/migration/block.c b/migration/block.c
index 0de9d84198..8e49382070 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -906,7 +906,7 @@ static int block_load(QEMUFile *f, void *opaque, int 
version_id)
 do {
 addr = qemu_get_be64(f);
 
-flags = addr & ~BDRV_SECTOR_MASK;
+flags = addr & (BDRV_SECTOR_SIZE - 1);
 addr >>= BDRV_SECTOR_BITS;
 
 if (flags & BLK_MIG_FLAG_DEVICE_BLOCK) {
-- 
2.21.0

Re: [Qemu-block] [PATCH v7 0/3] Fix qcow2+luks corruption introduced by commit 8ac0f15f335

[Maxim Levitsky]

On Mon, 2019-09-16 at 15:39 +0200, Max Reitz wrote:
> On 15.09.19 22:36, Maxim Levitsky wrote:
> > Commit 8ac0f15f335 accidently broke the COW of non changed areas
> > of newly allocated clusters, when the write spans multiple clusters,
> > and needs COW both prior and after the write.
> > This results in 'after' COW area being encrypted with wrong
> > sector address, which render it corrupted.
> > 
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922
> > 
> > CC: qemu-stable 
> > 
> > V2: grammar, spelling and code style fixes.
> > V3: more fixes after the review.
> > V4: addressed review comments from Max Reitz,
> > and futher refactored the qcow2_co_encrypt to just take full host and 
> > guest offset
> > which simplifies everything.
> > 
> > V5: reworked the patches so one of them fixes the bug
> > only and other one is just refactoring
> > 
> > V6: removed do_perform_cow_encrypt
> > 
> > V7: removed do_perform_cow_encrypt take two, this
> > time I hopefully did that correctly :-)
> > Also updated commit names and messages a bit
> 
> Luckily for you (maybe), Vladimir’s series doesn‘t quite pass the
> iotests for me, so unfortunately (I find it unfortunate) I had to remove
> it from my branch.  Thus, the conflicts are much more tame and I felt
> comfortable taking the series to my branch (with the remaining trivial
> conflicts resolved, and with Vladimir’s suggestion applied):
> 
> https://git.xanclic.moe/XanClic/qemu/commits/branch/block


First of all, Thanks!

I don't know if this is luckily for me since I already rebased my series on top 
of  
https://git.xanclic.moe/XanClic/qemu.git,
and run all qcow2 iotests, and only tests 
162 169 194 196 234 262 failed, and I know that 162 always fails
due to that kernel change I talked about here few days ago,
and rest for the AF_UNIX path len, which I need to do something
about in the long term. I sometimes do a separate build in 
directory which path doesn't trigger this, and sometimes,
when I know that I haven't done significant changes to the patches,
I just let these tests fail. In long term, maybe even in a few days
I'll allocate some time to rethink the build environment here to
fix that permanently.

Now I am rerunning the iotests just for fun, in short enough directory
to see if I can reproduce the failure that you had. After looking
in your report, that iotest 026 fails, it does pass here, but
then I am only running these iotests on my laptop so I probably
don't trigger the race you were able to.

So thanks again!
Best regards,
Maxim Levitsky

[Qemu-block] [PATCH 3/4] block/dirty-bitmap: drop BdrvDirtyBitmap.mutex

[Vladimir Sementsov-Ogievskiy]

mutex field is just a pointer to bs->dirty_bitmap_mutex, so no needs
to store it in BdrvDirtyBitmap when we have bs pointer in it (since
previous patch).

Drop mutex field. Constantly use bdrv_dirty_bitmaps_lock/unlock in
block/dirty-bitmap.c to make it more obvious that it's not per-bitmap
lock. Still, for simplicity, leave bdrv_dirty_bitmap_lock/unlock
functions as an external API.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/dirty-bitmap.c | 84 +---
 1 file changed, 41 insertions(+), 43 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index f3dc7b3ca5..76a8e59e61 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -28,7 +28,6 @@
 #include "block/blockjob.h"
 
 struct BdrvDirtyBitmap {
-QemuMutex *mutex;
 BlockDriverState *bs;
 HBitmap *bitmap;/* Dirty bitmap implementation */
 bool busy;  /* Bitmap is busy, it can't be used via QMP */
@@ -71,12 +70,12 @@ static inline void 
bdrv_dirty_bitmaps_unlock(BlockDriverState *bs)
 
 void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap)
 {
-qemu_mutex_lock(bitmap->mutex);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 }
 
 void bdrv_dirty_bitmap_unlock(BdrvDirtyBitmap *bitmap)
 {
-qemu_mutex_unlock(bitmap->mutex);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 /* Called with BQL or dirty_bitmap lock taken.  */
@@ -116,7 +115,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
*bs,
 }
 bitmap = g_new0(BdrvDirtyBitmap, 1);
 bitmap->bs = bs;
-bitmap->mutex = &bs->dirty_bitmap_mutex;
 bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(granularity));
 bitmap->size = bitmap_size;
 bitmap->name = g_strdup(name);
@@ -150,9 +148,9 @@ static bool bdrv_dirty_bitmap_busy(const BdrvDirtyBitmap 
*bitmap)
 
 void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy)
 {
-qemu_mutex_lock(bitmap->mutex);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 bitmap->busy = busy;
-qemu_mutex_unlock(bitmap->mutex);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 /* Called with BQL taken.  */
@@ -277,10 +275,10 @@ void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap 
*bitmap)
 /* Called with BQL taken. */
 void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap)
 {
-assert(bitmap->mutex == bitmap->successor->mutex);
-qemu_mutex_lock(bitmap->mutex);
+assert(bitmap->bs == bitmap->successor->bs);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 bdrv_enable_dirty_bitmap_locked(bitmap->successor);
-qemu_mutex_unlock(bitmap->mutex);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 /* Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.  */
@@ -360,9 +358,9 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BdrvDirtyBitmap 
*parent,
 {
 BdrvDirtyBitmap *ret;
 
-qemu_mutex_lock(parent->mutex);
+bdrv_dirty_bitmaps_lock(parent->bs);
 ret = bdrv_reclaim_dirty_bitmap_locked(parent, errp);
-qemu_mutex_unlock(parent->mutex);
+bdrv_dirty_bitmaps_unlock(parent->bs);
 
 return ret;
 }
@@ -434,16 +432,16 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState 
*bs,
 
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
-bdrv_dirty_bitmap_lock(bitmap);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 bitmap->disabled = true;
-bdrv_dirty_bitmap_unlock(bitmap);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
-bdrv_dirty_bitmap_lock(bitmap);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 bdrv_enable_dirty_bitmap_locked(bitmap);
-bdrv_dirty_bitmap_unlock(bitmap);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
@@ -484,9 +482,9 @@ bool bdrv_dirty_bitmap_get_locked(BdrvDirtyBitmap *bitmap, 
int64_t offset)
 bool bdrv_dirty_bitmap_get(BdrvDirtyBitmap *bitmap, int64_t offset)
 {
 bool ret;
-bdrv_dirty_bitmap_lock(bitmap);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 ret = bdrv_dirty_bitmap_get_locked(bitmap, offset);
-bdrv_dirty_bitmap_unlock(bitmap);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 
 return ret;
 }
@@ -551,9 +549,9 @@ void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
 void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t offset, int64_t bytes)
 {
-bdrv_dirty_bitmap_lock(bitmap);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 bdrv_set_dirty_bitmap_locked(bitmap, offset, bytes);
-bdrv_dirty_bitmap_unlock(bitmap);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 /* Called within bdrv_dirty_bitmap_lock..unlock */
@@ -567,15 +565,15 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap 
*bitmap,
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
  int64_t offset, int64_t bytes)
 {
-bdrv_dirty_bitmap_lock(bitmap);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 bdrv_reset_dirty_bitmap_locked(bitmap, offset

[Qemu-block] [PATCH 0/4] bitmaps: some refactoring

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here I suggest some refactoring patches for bitmaps.

Vladimir Sementsov-Ogievskiy (4):
  block/dirty-bitmap: drop meta
  block/dirty-bitmap: add bs link
  block/dirty-bitmap: drop BdrvDirtyBitmap.mutex
  block/dirty-bitmap: refactor bdrv_dirty_bitmap_next

 include/block/dirty-bitmap.h   |  28 +++---
 block.c|   4 +-
 block/backup.c |  14 ++-
 block/dirty-bitmap.c   | 165 -
 block/mirror.c |   4 +-
 block/qcow2-bitmap.c   |  14 +--
 blockdev.c |   6 +-
 migration/block-dirty-bitmap.c |  11 +--
 migration/block.c  |   4 +-
 9 files changed, 95 insertions(+), 155 deletions(-)

-- 
2.21.0

Re: [Qemu-block] [PATCH v7 0/3] Fix qcow2+luks corruption introduced by commit 8ac0f15f335

[Max Reitz]

On 15.09.19 22:36, Maxim Levitsky wrote:
> Commit 8ac0f15f335 accidently broke the COW of non changed areas
> of newly allocated clusters, when the write spans multiple clusters,
> and needs COW both prior and after the write.
> This results in 'after' COW area being encrypted with wrong
> sector address, which render it corrupted.
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922
> 
> CC: qemu-stable 
> 
> V2: grammar, spelling and code style fixes.
> V3: more fixes after the review.
> V4: addressed review comments from Max Reitz,
> and futher refactored the qcow2_co_encrypt to just take full host and 
> guest offset
> which simplifies everything.
> 
> V5: reworked the patches so one of them fixes the bug
> only and other one is just refactoring
> 
> V6: removed do_perform_cow_encrypt
> 
> V7: removed do_perform_cow_encrypt take two, this
> time I hopefully did that correctly :-)
> Also updated commit names and messages a bit

Luckily for you (maybe), Vladimir’s series doesn‘t quite pass the
iotests for me, so unfortunately (I find it unfortunate) I had to remove
it from my branch.  Thus, the conflicts are much more tame and I felt
comfortable taking the series to my branch (with the remaining trivial
conflicts resolved, and with Vladimir’s suggestion applied):

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

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v4 0/5] qcow2: async handling of fragmented io

[Max Reitz]

On 13.09.19 10:58, Max Reitz wrote:
> On 16.08.19 17:30, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here is an asynchronous scheme for handling fragmented qcow2
>> reads and writes. Both qcow2 read and write functions loops through
>> sequential portions of data. The series aim it to parallelize these
>> loops iterations.
>> It improves performance for fragmented qcow2 images, I've tested it
>> as described below.
> 
> Thanks, I’ve changed two things:
> - Replaced assert((x & (BDRV_SECTOR_SIZE - 1)) == 0) by
>   assert(QEMU_IS_ALIGNED(x, BDRV_SECTOR_SIZE)) in patch 3 (conflict with
>   “block: Use QEMU_IS_ALIGNED”), and
> - Replaced the remaining instance of “qcow2_co_do_pwritev()” by
>   “qcow2_co_pwritev_task()” in a comment in patch 4
> 
> and applied the series to my block branch:
> 
> https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Unfortunately, I’ll have to unstage the series for now because the fix
to 026’s reference output isn’t stable.

When running the test in parallel (I can reproduce it with four
instances on my machine with two cores + HT), I get failures like:

026  fail   [15:21:09] [15:21:37]  (last: 18s)   output
mismatch (see 026.out.bad)
--- tests/qemu-iotests/026.out 2019-09-16 14:49:20.720410701 +0200
+++ tests/qemu-iotests/026.out.bad   2019-09-16 15:21:37.180711936 +0200
@@ -563,7 +563,7 @@
 qemu-io: Failed to flush the refcount block cache: No space left on device
 write failed: No space left on device

-74 leaked clusters were found on the image.
+522 leaked clusters were found on the image.
 This means waste of disk space, but no harm to data.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824

Failures: 026
Failed 1 of 1 iotests

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v7 3/3] qemu-iotests: Add test for bz #1745922

[Max Reitz]

On 15.09.19 22:36, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky 
> Tested-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/263 | 91 ++
>  tests/qemu-iotests/263.out | 40 +
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 132 insertions(+)
>  create mode 100755 tests/qemu-iotests/263
>  create mode 100644 tests/qemu-iotests/263.out

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v7 2/3] block/qcow2: refactor encryption code

[Max Reitz]

On 15.09.19 22:36, Maxim Levitsky wrote:
> * Change the qcow2_co_{encrypt|decrypt} to just receive full host and
>   guest offsets and use this function directly instead of calling
>   do_perform_cow_encrypt (which is removed by that patch).
> 
> * Adjust qcow2_co_encdec to take full host and guest offsets as well.
> 
> * Document the qcow2_co_{encrypt|decrypt} arguments
>   to prevent the bug fixed in former commit from hopefully
>   happening again.
> 
> Signed-off-by: Maxim Levitsky 
> ---
>  block/qcow2-cluster.c | 41 ++--
>  block/qcow2-threads.c | 63 +--
>  block/qcow2.c |  5 ++--
>  block/qcow2.h |  8 +++---
>  4 files changed, 70 insertions(+), 47 deletions(-)

Looks good to me, but there are conflicts with two series I’ve already
taken to my branch: Namely “qcow2: async handling of fragmented io” by
Vladimir and “Alignment checks cleanup​” by Nir.

Unfortunately, those conflicts (while not difficult to resolve) are not
quite trivial, so I’d rather not resolve them myself.

I’ll send a pull request today, but until they are in master, you could
rebase on my branch (either of
- https://git.xanclic.moe/XanClic/qemu.git block
- https://github.com/XanClic/qemu.git block
, whichever you prefer that works).

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v7 1/3] block/qcow2: Fix corruption introduced by commit 8ac0f15f335

[Max Reitz]

On 15.09.19 22:36, Maxim Levitsky wrote:
> This fixes subtle corruption introduced by luks threaded encryption
> in commit 8ac0f15f335
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922
> 
> The corruption happens when we do a write that
>* writes to two or more unallocated clusters at once
>* doesn't fully cover the first sector
>* doesn't fully cover the last sector
>* uses luks encryption
> 
> In this case, when allocating the new clusters we COW both areas
> prior to the write and after the write, and we encrypt them.
> 
> The above mentioned commit accidentally made it so we encrypt the
> second COW area using the physical cluster offset of the first area.
> 
> The problem is that offset_in_cluster in do_perform_cow_encrypt
> can be larger that the cluster size, thus cluster_offset
> will no longer point to the start of the cluster at which encrypted
> area starts.
> 
> Next patch in this series will refactor the code to avoid all these
> assumptions.
> 
> In the bugreport that was triggered by rebasing a luks image to new,
> zero filled base, which lot of such writes, and causes some files
> with zero areas to contain garbage there instead.
> But as described above it can happen elsewhere as well
> 
> 
> Signed-off-by: Maxim Levitsky 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/qcow2-cluster.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v3] virtio-blk: schedule virtio_notify_config to run on main context

[Sergio Lopez]

virtio_notify_config() needs to acquire the global mutex, which isn't
allowed from an iothread, and may lead to a deadlock like this:

 - main thead
  * Has acquired: qemu_global_mutex.
  * Is trying the acquire: iothread AioContext lock via
AIO_WAIT_WHILE (after aio_poll).

 - iothread
  * Has acquired: AioContext lock.
  * Is trying to acquire: qemu_global_mutex (via
virtio_notify_config->prepare_mmio_access).

If virtio_blk_resize() is called from an iothread, schedule
virtio_notify_config() to be run in the main context BH.

Signed-off-by: Sergio Lopez 
---
Changelog

v3:
 - Unconditionally schedule the work to be done in the main context BH
   (thanks John Snow and Kevin Wolf).

v2:
 - Use aio_bh_schedule_oneshot instead of scheduling a coroutine
   (thanks Kevin Wolf).
 - Switch from RFC to v2 patch.
---
 hw/block/virtio-blk.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 18851601cb..0163285f6f 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -16,6 +16,7 @@
 #include "qemu/iov.h"
 #include "qemu/module.h"
 #include "qemu/error-report.h"
+#include "qemu/main-loop.h"
 #include "trace.h"
 #include "hw/block/block.h"
 #include "hw/qdev-properties.h"
@@ -1086,11 +1087,25 @@ static int virtio_blk_load_device(VirtIODevice *vdev, 
QEMUFile *f,
 return 0;
 }
 
+static void virtio_resize_cb(void *opaque)
+{
+VirtIODevice *vdev = opaque;
+
+assert(qemu_get_current_aio_context() == qemu_get_aio_context());
+virtio_notify_config(vdev);
+}
+
 static void virtio_blk_resize(void *opaque)
 {
 VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
 
-virtio_notify_config(vdev);
+/*
+ * virtio_notify_config() needs to acquire the global mutex,
+ * so it can't be called from an iothread. Instead, schedule
+ * it to be run in the main context BH.
+ */
+aio_bh_schedule_oneshot(qemu_get_aio_context(),
+virtio_resize_cb, vdev);
 }
 
 static const BlockDevOps virtio_block_ops = {
-- 
2.21.0

Re: [Qemu-block] [PATCH v2 1/2] blockdev: release the AioContext at drive_backup_prepare

[Sergio Lopez]

Kevin Wolf  writes:

> Am 13.09.2019 um 21:54 hat John Snow geschrieben:
>> 
>> 
>> On 9/13/19 11:25 AM, Sergio Lopez wrote:
>> > do_drive_backup() already acquires the AioContext, so release it
>> > before the call.
>> > 
>> > Signed-off-by: Sergio Lopez 
>> > ---
>> >  blockdev.c | 6 +-
>> >  1 file changed, 1 insertion(+), 5 deletions(-)
>> > 
>> > diff --git a/blockdev.c b/blockdev.c
>> > index fbef6845c8..3927fdab80 100644
>> > --- a/blockdev.c
>> > +++ b/blockdev.c
>> > @@ -1783,20 +1783,16 @@ static void drive_backup_prepare(BlkActionState 
>> > *common, Error **errp)
>> >  
>> >  aio_context = bdrv_get_aio_context(bs);
>> >  aio_context_acquire(aio_context);
>> > -
>
> Are you removing this unrelated empty line intentionally?

Yes. In the sense of that whole set of lines being a "open drained
section" block.

>> >  /* Paired with .clean() */
>> >  bdrv_drained_begin(bs);
>> 
>> Do we need to make this change to blockdev_backup_prepare as well?
>
> Actually, the whole structure feels a bit wrong. We get the bs here and
> take its lock, then release the lock again and forget the reference,
> only to do both things again inside do_drive_backup().
>
> The way snapshots work is that the "normal" snapshot commands are
> wrappers that turn it into a single-entry transaction. Then you have
> only one code path where you can resolve the ID and take the lock just
> once. So maybe backup should work like this, too?

I'm neither opposed nor in favor, but I think this is outside the scope
of this patch series.

Sergio.


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v2 1/2] blockdev: release the AioContext at drive_backup_prepare

[Sergio Lopez]

John Snow  writes:

> On 9/13/19 11:25 AM, Sergio Lopez wrote:
>> do_drive_backup() already acquires the AioContext, so release it
>> before the call.
>> 
>> Signed-off-by: Sergio Lopez 
>> ---
>>  blockdev.c | 6 +-
>>  1 file changed, 1 insertion(+), 5 deletions(-)
>> 
>> diff --git a/blockdev.c b/blockdev.c
>> index fbef6845c8..3927fdab80 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1783,20 +1783,16 @@ static void drive_backup_prepare(BlkActionState 
>> *common, Error **errp)
>>  
>>  aio_context = bdrv_get_aio_context(bs);
>>  aio_context_acquire(aio_context);
>> -
>>  /* Paired with .clean() */
>>  bdrv_drained_begin(bs);
>
> Do we need to make this change to blockdev_backup_prepare as well?

Yes, we do. Thanks.

>> +aio_context_release(aio_context);
>>  
>>  state->bs = bs;
>>  
>>  state->job = do_drive_backup(backup, common->block_job_txn, &local_err);
>>  if (local_err) {
>>  error_propagate(errp, local_err);
>> -goto out;
>>  }
>> -
>> -out:
>> -aio_context_release(aio_context);
>>  }
>>  
>>  static void drive_backup_commit(BlkActionState *common)
>> 



signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v6 25/42] mirror: Deal with filters

[Max Reitz]

On 13.09.19 14:55, Kevin Wolf wrote:
> Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
>> This includes some permission limiting (for example, we only need to
>> take the RESIZE permission for active commits where the base is smaller
>> than the top).
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  block/mirror.c | 117 ++---
>>  blockdev.c |  47 +---
>>  2 files changed, 131 insertions(+), 33 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 54bafdf176..6ddbfb9708 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -42,6 +42,7 @@ typedef struct MirrorBlockJob {
>>  BlockBackend *target;
>>  BlockDriverState *mirror_top_bs;
>>  BlockDriverState *base;
>> +BlockDriverState *base_overlay;
>>  
>>  /* The name of the graph node to replace */
>>  char *replaces;
>> @@ -665,8 +666,10 @@ static int mirror_exit_common(Job *job)
>>   &error_abort);
>>  if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>>  BlockDriverState *backing = s->is_none_mode ? src : s->base;
>> -if (backing_bs(target_bs) != backing) {
>> -bdrv_set_backing_hd(target_bs, backing, &local_err);
>> +BlockDriverState *unfiltered_target = 
>> bdrv_skip_rw_filters(target_bs);
>> +
>> +if (bdrv_filtered_cow_bs(unfiltered_target) != backing) {
>> +bdrv_set_backing_hd(unfiltered_target, backing, &local_err);
>>  if (local_err) {
>>  error_report_err(local_err);
>>  ret = -EPERM;
>> @@ -715,7 +718,7 @@ static int mirror_exit_common(Job *job)
>>   * valid.
>>   */
>>  block_job_remove_all_bdrv(bjob);
>> -bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), 
>> &error_abort);
>> +bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs, 
>> &error_abort);
>>  
>>  /* We just changed the BDS the job BB refers to (with either or both of 
>> the
>>   * bdrv_replace_node() calls), so switch the BB back so the cleanup does
>> @@ -812,7 +815,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob 
>> *s)
>>  return 0;
>>  }
>>  
>> -ret = bdrv_is_allocated_above(bs, base, false, offset, bytes, 
>> &count);
>> +ret = bdrv_is_allocated_above(bs, s->base_overlay, true, offset, 
>> bytes,
>> +  &count);
>>  if (ret < 0) {
>>  return ret;
>>  }
>> @@ -908,7 +912,7 @@ static int coroutine_fn mirror_run(Job *job, Error 
>> **errp)
>>  } else {
>>  s->target_cluster_size = BDRV_SECTOR_SIZE;
>>  }
>> -if (backing_filename[0] && !target_bs->backing &&
>> +if (backing_filename[0] && !bdrv_backing_chain_next(target_bs) &&
>>  s->granularity < s->target_cluster_size) {
>>  s->buf_size = MAX(s->buf_size, s->target_cluster_size);
>>  s->cow_bitmap = bitmap_new(length);
>> @@ -1088,8 +1092,9 @@ static void mirror_complete(Job *job, Error **errp)
>>  if (s->backing_mode == MIRROR_OPEN_BACKING_CHAIN) {
>>  int ret;
>>  
>> -assert(!target->backing);
>> -ret = bdrv_open_backing_file(target, NULL, "backing", errp);
>> +assert(!bdrv_backing_chain_next(target));
>> +ret = bdrv_open_backing_file(bdrv_skip_rw_filters(target), NULL,
>> + "backing", errp);
>>  if (ret < 0) {
>>  return;
>>  }
>> @@ -1531,8 +1536,8 @@ static BlockJob *mirror_start_job(
>>  MirrorBlockJob *s;
>>  MirrorBDSOpaque *bs_opaque;
>>  BlockDriverState *mirror_top_bs;
>> -bool target_graph_mod;
>>  bool target_is_backing;
>> +uint64_t target_perms, target_shared_perms;
>>  Error *local_err = NULL;
>>  int ret;
>>  
>> @@ -1551,7 +1556,7 @@ static BlockJob *mirror_start_job(
>>  buf_size = DEFAULT_MIRROR_BUF_SIZE;
>>  }
>>  
>> -if (bs == target) {
>> +if (bdrv_skip_rw_filters(bs) == bdrv_skip_rw_filters(target)) {
>>  error_setg(errp, "Can't mirror node into itself");
>>  return NULL;
>>  }
>> @@ -1615,15 +1620,50 @@ static BlockJob *mirror_start_job(
>>   * In the case of active commit, things look a bit different, though,
>>   * because the target is an already populated backing file in active 
>> use.
>>   * We can allow anything except resize there.*/
>> +
>> +target_perms = BLK_PERM_WRITE;
>> +target_shared_perms = BLK_PERM_WRITE_UNCHANGED;
>> +
>>  target_is_backing = bdrv_chain_contains(bs, target);
>> -target_graph_mod = (backing_mode != MIRROR_LEAVE_BACKING_CHAIN);
>> +if (target_is_backing) {
>> +int64_t bs_size, target_size;
>> +bs_size = bdrv_getlength(bs);
>> +if (bs_size < 0) {
>> +error_setg_errno(errp, -bs_size,
>> + "Could not inquire top image size");
>> +goto fail;

[Qemu-block] [PATCH v2 1/2] trace: Remove trailing newline in events

[Philippe Mathieu-Daudé]

While the tracing frawework does not forbid trailing newline in
events format string, using them lead to confuse output.
It is the responsibility of the backend to properly end an event
line.

Some of our formats have trailing newlines, remove them.

Reviewed-by: John Snow 
Reviewed-by: Kevin Wolf 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/misc/trace-events | 10 +-
 hw/scsi/trace-events |  2 +-
 hw/sd/trace-events   |  2 +-
 nbd/trace-events |  4 ++--
 net/trace-events |  6 +++---
 5 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index c1ea1aa437..74276225f8 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -118,11 +118,11 @@ iotkit_secctl_ns_read(uint32_t offset, uint64_t data, 
unsigned size) "IoTKit Sec
 iotkit_secctl_ns_write(uint32_t offset, uint64_t data, unsigned size) "IoTKit 
SecCtl NS regs write: offset 0x%x data 0x%" PRIx64 " size %u"
 
 # imx6ul_ccm.c
-ccm_entry(void) "\n"
-ccm_freq(uint32_t freq) "freq = %d\n"
-ccm_clock_freq(uint32_t clock, uint32_t freq) "(Clock = %d) = %d\n"
-ccm_read_reg(const char *reg_name, uint32_t value) "reg[%s] <= 0x%" PRIx32 "\n"
-ccm_write_reg(const char *reg_name, uint32_t value) "reg[%s] => 0x%" PRIx32 
"\n"
+ccm_entry(void) ""
+ccm_freq(uint32_t freq) "freq = %d"
+ccm_clock_freq(uint32_t clock, uint32_t freq) "(Clock = %d) = %d"
+ccm_read_reg(const char *reg_name, uint32_t value) "reg[%s] <= 0x%" PRIx32
+ccm_write_reg(const char *reg_name, uint32_t value) "reg[%s] => 0x%" PRIx32
 
 # iotkit-sysinfo.c
 iotkit_sysinfo_read(uint64_t offset, uint64_t data, unsigned size) "IoTKit 
SysInfo read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
diff --git a/hw/scsi/trace-events b/hw/scsi/trace-events
index 452b5994e6..b0820052f8 100644
--- a/hw/scsi/trace-events
+++ b/hw/scsi/trace-events
@@ -28,7 +28,7 @@ mptsas_mmio_read(void *dev, uint32_t addr, uint32_t val) "dev 
%p addr 0x%08x val
 mptsas_mmio_unhandled_read(void *dev, uint32_t addr) "dev %p addr 0x%08x"
 mptsas_mmio_unhandled_write(void *dev, uint32_t addr, uint32_t val) "dev %p 
addr 0x%08x value 0x%x"
 mptsas_mmio_write(void *dev, uint32_t addr, uint32_t val) "dev %p addr 0x%08x 
value 0x%x"
-mptsas_process_message(void *dev, int msg, uint32_t ctx) "dev %p cmd %d 
context 0x%08x\n"
+mptsas_process_message(void *dev, int msg, uint32_t ctx) "dev %p cmd %d 
context 0x%08x"
 mptsas_process_scsi_io_request(void *dev, int bus, int target, int lun, 
uint64_t len) "dev %p dev %d:%d:%d length %"PRIu64""
 mptsas_reset(void *dev) "dev %p "
 mptsas_scsi_overflow(void *dev, uint32_t ctx, uint64_t req, uint64_t found) 
"dev %p context 0x%08x: %"PRIu64"/%"PRIu64""
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 52971dc033..efcff666a2 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -4,7 +4,7 @@
 bcm2835_sdhost_read(uint64_t offset, uint64_t data, unsigned size) "offset 
0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 bcm2835_sdhost_write(uint64_t offset, uint64_t data, unsigned size) "offset 
0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 bcm2835_sdhost_edm_change(const char *why, uint32_t edm) "(%s) EDM now 0x%x"
-bcm2835_sdhost_update_irq(uint32_t irq) "IRQ bits 0x%x\n"
+bcm2835_sdhost_update_irq(uint32_t irq) "IRQ bits 0x%x"
 
 # core.c
 sdbus_command(const char *bus_name, uint8_t cmd, uint32_t arg) "@%s CMD%02d 
arg 0x%08x"
diff --git a/nbd/trace-events b/nbd/trace-events
index f6cde96790..a955918e97 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -61,8 +61,8 @@ nbd_negotiate_begin(void) "Beginning negotiation"
 nbd_negotiate_new_style_size_flags(uint64_t size, unsigned flags) "advertising 
size %" PRIu64 " and flags 0x%x"
 nbd_negotiate_success(void) "Negotiation succeeded"
 nbd_receive_request(uint32_t magic, uint16_t flags, uint16_t type, uint64_t 
from, uint32_t len) "Got request: { magic = 0x%" PRIx32 ", .flags = 0x%" PRIx16 
", .type = 0x%" PRIx16 ", from = %" PRIu64 ", len = %" PRIu32 " }"
-nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching 
clients to AIO context %p\n"
-nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients 
from AIO context %p\n"
+nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching 
clients to AIO context %p"
+nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients 
from AIO context %p"
 nbd_co_send_simple_reply(uint64_t handle, uint32_t error, const char *errname, 
int len) "Send simple reply: handle = %" PRIu64 ", error = %" PRIu32 " (%s), 
len = %d"
 nbd_co_send_structured_done(uint64_t handle) "Send structured reply done: 
handle = %" PRIu64
 nbd_co_send_structured_read(uint64_t handle, uint64_t offset, void *data, 
size_t size) "Send structured read data reply: handle = %" PRIu64 ", offset = 
%" PRIu64 ", data = %p, len = %zu"
diff --git a/net/trace-events b/net/trace-events
index ac57056497..02c13fd0ba 100644
--- a/net/trace-events
+++ b/net/trace-events
@@ -17,9 +17,9 @@ colo_compare_icmp_m

Re: [Qemu-block] [PATCH v6 28/42] stream: Deal with filters

[Max Reitz]

On 13.09.19 16:16, Kevin Wolf wrote:
> Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
>> Because of the recent changes that make the stream job independent of
>> the base node and instead track the node above it, we have to split that
>> "bottom" node into two cases: The bottom COW node, and the node directly
>> above the base node (which may be an R/W filter or the bottom COW node).
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  qapi/block-core.json |  4 
>>  block/stream.c   | 52 
>>  blockdev.c   |  2 +-
>>  3 files changed, 38 insertions(+), 20 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 38c4dbd7c3..3c54717870 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2516,6 +2516,10 @@
>>  # On successful completion the image file is updated to drop the backing 
>> file
>>  # and the BLOCK_JOB_COMPLETED event is emitted.
>>  #
>> +# In case @device is a filter node, block-stream modifies the first 
>> non-filter
>> +# overlay node below it to point to base's backing node (or NULL if @base 
>> was
>> +# not specified) instead of modifying @device itself.
>> +#
>>  # @job-id: identifier for the newly-created block job. If
>>  #  omitted, the device name will be used. (Since 2.7)
>>  #
>> diff --git a/block/stream.c b/block/stream.c
>> index 4c8b89884a..bd4a351dae 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>> @@ -31,7 +31,8 @@ enum {
>>  
>>  typedef struct StreamBlockJob {
>>  BlockJob common;
>> -BlockDriverState *bottom;
>> +BlockDriverState *bottom_cow_node;
>> +BlockDriverState *above_base;
> 
> Confusing naming, especially because in commit you used above_base for
> what is bottom_cow_node here. Vladimir already suggested using
> base_overlay consistently, so we should do this here too (for
> bottom_cow_node). above_base can keep its name because the different
> above_base in commit is going to be renamed).

Sure.

>>  BlockdevOnError on_error;
>>  char *backing_file_str;
>>  bool bs_read_only;
>> @@ -54,7 +55,7 @@ static void stream_abort(Job *job)
>>  
>>  if (s->chain_frozen) {
>>  BlockJob *bjob = &s->common;
>> -bdrv_unfreeze_chain(blk_bs(bjob->blk), s->bottom);
>> +bdrv_unfreeze_chain(blk_bs(bjob->blk), s->above_base);
>>  }
>>  }
>>  
>> @@ -63,14 +64,15 @@ static int stream_prepare(Job *job)
>>  StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>>  BlockJob *bjob = &s->common;
>>  BlockDriverState *bs = blk_bs(bjob->blk);
>> -BlockDriverState *base = backing_bs(s->bottom);
>> +BlockDriverState *unfiltered_bs = bdrv_skip_rw_filters(bs);
>> +BlockDriverState *base = bdrv_filtered_bs(s->above_base);
>>  Error *local_err = NULL;
>>  int ret = 0;
>>  
>> -bdrv_unfreeze_chain(bs, s->bottom);
>> +bdrv_unfreeze_chain(bs, s->above_base);
>>  s->chain_frozen = false;
>>  
>> -if (bs->backing) {
>> +if (bdrv_filtered_cow_child(unfiltered_bs)) {
>>  const char *base_id = NULL, *base_fmt = NULL;
>>  if (base) {
>>  base_id = s->backing_file_str;
>> @@ -78,8 +80,8 @@ static int stream_prepare(Job *job)
>>  base_fmt = base->drv->format_name;
>>  }
>>  }
>> -bdrv_set_backing_hd(bs, base, &local_err);
>> -ret = bdrv_change_backing_file(bs, base_id, base_fmt);
>> +bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
>> +ret = bdrv_change_backing_file(unfiltered_bs, base_id, base_fmt);
>>  if (local_err) {
>>  error_report_err(local_err);
>>  return -EPERM;
>> @@ -110,7 +112,8 @@ static int coroutine_fn stream_run(Job *job, Error 
>> **errp)
>>  StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>>  BlockBackend *blk = s->common.blk;
>>  BlockDriverState *bs = blk_bs(blk);
>> -bool enable_cor = !backing_bs(s->bottom);
>> +BlockDriverState *unfiltered_bs = bdrv_skip_rw_filters(bs);
>> +bool enable_cor = !bdrv_filtered_bs(s->above_base);
>>  int64_t len;
>>  int64_t offset = 0;
>>  uint64_t delay_ns = 0;
>> @@ -119,7 +122,7 @@ static int coroutine_fn stream_run(Job *job, Error 
>> **errp)
>>  int64_t n = 0; /* bytes */
>>  void *buf;
>>  
>> -if (bs == s->bottom) {
>> +if (unfiltered_bs == s->bottom_cow_node) {
>>  /* Nothing to stream */
>>  return 0;
>>  }
>> @@ -154,13 +157,14 @@ static int coroutine_fn stream_run(Job *job, Error 
>> **errp)
>>  
>>  copy = false;
>>  
>> -ret = bdrv_is_allocated(bs, offset, STREAM_BUFFER_SIZE, &n);
>> +ret = bdrv_is_allocated(unfiltered_bs, offset, STREAM_BUFFER_SIZE, 
>> &n);
>>  if (ret == 1) {
>>  /* Allocated in the top, no need to copy.  */
>>  } else if (ret >= 0) {
>>  /* Copy if allocated in the intermediate images.  Limit to the
>

[Qemu-block] [PATCH v2 0/2] trace: Forbid trailing newline in event format

[Philippe Mathieu-Daudé]

Hi Stefan,

I'v been confused by trailing newline in trace reports,
so this series aims to fix this, by cleaning current
formats and add a check to catch new one introduced.

v2:
- Use regex format (easier to review)
- Added R-b

Regards,

Phil.

Philippe Mathieu-Daudé (2):
  trace: Remove trailing newline in events
  trace: Forbid event format ending with newline character

 docs/devel/tracing.txt|  2 ++
 hw/misc/trace-events  | 10 +-
 hw/scsi/trace-events  |  2 +-
 hw/sd/trace-events|  2 +-
 nbd/trace-events  |  4 ++--
 net/trace-events  |  6 +++---
 scripts/tracetool/__init__.py |  3 +++
 7 files changed, 17 insertions(+), 12 deletions(-)

-- 
2.20.1

[Qemu-block] [PATCH v2 2/2] trace: Forbid event format ending with newline character

[Philippe Mathieu-Daudé]

Event format ending with newlines confuse the trace reports.
Forbid them.

Add a check to refuse new format added with trailing newline:

  $ make
  [...]
GEN hw/misc/trace.h
  Traceback (most recent call last):
File "scripts/tracetool.py", line 152, in 
  main(sys.argv)
File "scripts/tracetool.py", line 143, in main
  events.extend(tracetool.read_events(fh, arg))
File "scripts/tracetool/__init__.py", line 367, in read_events
  event = Event.build(line)
File "scripts/tracetool/__init__.py", line 281, in build
  raise ValueError("Event format can not end with a newline character")
  ValueError: Error at hw/misc/trace-events:121: Event format can not end with 
a newline character

Reviewed-by: John Snow 
Reviewed-by: Kevin Wolf 
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: "\\n\"" -> r'\n"' (jsnow)
---
 docs/devel/tracing.txt| 2 ++
 scripts/tracetool/__init__.py | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/docs/devel/tracing.txt b/docs/devel/tracing.txt
index 76e492a489..8231bbf5d1 100644
--- a/docs/devel/tracing.txt
+++ b/docs/devel/tracing.txt
@@ -112,6 +112,8 @@ Trace events should use types as follows:
 Format strings should reflect the types defined in the trace event.  Take
 special care to use PRId64 and PRIu64 for int64_t and uint64_t types,
 respectively.  This ensures portability between 32- and 64-bit platforms.
+Format strings must not end with a newline character.  It is the responsibility
+of backends to adapt line ending for proper logging.
 
 Each event declaration will start with the event name, then its arguments,
 finally a format string for pretty-printing. For example:
diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 6fca674936..04279fa62e 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -277,6 +277,9 @@ class Event(object):
 if fmt.find("%m") != -1 or fmt_trans.find("%m") != -1:
 raise ValueError("Event format '%m' is forbidden, pass the error "
  "as an explicit trace argument")
+if fmt.endswith(r'\n"'):
+raise ValueError("Event format must not end with a newline "
+ "character")
 
 if len(fmt_trans) > 0:
 fmt = [fmt_trans, fmt]
-- 
2.20.1

Re: [Qemu-block] [PATCH v7 2/3] block/qcow2: refactor encryption code

[Vladimir Sementsov-Ogievskiy]

15.09.2019 23:36, Maxim Levitsky wrote:
> * Change the qcow2_co_{encrypt|decrypt} to just receive full host and
>guest offsets and use this function directly instead of calling
>do_perform_cow_encrypt (which is removed by that patch).
> 
> * Adjust qcow2_co_encdec to take full host and guest offsets as well.
> 
> * Document the qcow2_co_{encrypt|decrypt} arguments
>to prevent the bug fixed in former commit from hopefully
>happening again.
> 
> Signed-off-by: Maxim Levitsky 
> ---
>   block/qcow2-cluster.c | 41 ++--
>   block/qcow2-threads.c | 63 +--
>   block/qcow2.c |  5 ++--
>   block/qcow2.h |  8 +++---
>   4 files changed, 70 insertions(+), 47 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index bfeb0241d7..a2d4909024 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c

[..]

>   static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
>uint64_t cluster_offset,
>unsigned offset_in_cluster,
> @@ -891,11 +869,20 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta 
> *m)
>   
>   /* Encrypt the data if necessary before writing it */
>   if (bs->encrypted) {
> -if (!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> -start->offset, start_buffer,
> -start->nb_bytes) ||
> -!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> -end->offset, end_buffer, end->nb_bytes)) 
> {
> +ret = qcow2_co_encrypt(bs,
> +   m->alloc_offset + start->offset,
> +   m->offset + start->offset,
> +   start_buffer, start->nb_bytes);
> +if (ret < 0) {
> +ret = -EIO;
> +goto fail;

Just go to fail I think, don't reassign ret variable.

> +}
> +
> +ret = qcow2_co_encrypt(bs,
> +   m->alloc_offset + end->offset,
> +   m->offset + end->offset,
> +   end_buffer, end->nb_bytes);
> +if (ret < 0) {
>   ret = -EIO;

and here.

with these two places fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy 

(I think, these simple changes may be applied while queuing, so you don't
need to resend, if there no other reasons)




-- 
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PATCH v2 2/3] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

14.09.2019 1:01, John Snow wrote:
> 
> 
> On 9/11/19 11:00 AM, Vladimir Sementsov-Ogievskiy wrote:
>> It's more comfortable to not deal with local_err.
>>
> 
> I agree.
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   block/qcow2.h|  5 ++---
>>   include/block/block_int.h|  6 +++---
>>   include/block/dirty-bitmap.h |  5 ++---
>>   block/dirty-bitmap.c |  9 +
>>   block/qcow2-bitmap.c | 20 +++-
>>   blockdev.c   |  7 +++
>>   6 files changed, 26 insertions(+), 26 deletions(-)
>>
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 998bcdaef1..99ee88f802 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -747,9 +747,8 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState 
>> *bs,
>> const char *name,
>> uint32_t granularity,
>> Error **errp);
>> -void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> -  const char *name,
>> -  Error **errp);
>> +int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char 
>> *name,
>> + Error **errp);
>>   
>>   ssize_t coroutine_fn
>>   qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 0422acdf1c..503ac9e3cd 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -556,9 +556,9 @@ struct BlockDriver {
>>   const char *name,
>>   uint32_t granularity,
>>   Error **errp);
>> -void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
>> -const char *name,
>> -Error **errp);
>> +int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
>> +   const char *name,
>> +   Error **errp);
>>   
>>   /**
>>* Register/unregister a buffer for I/O. For example, when the driver 
>> is
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index 4b4b731b46..07503b03b5 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -37,9 +37,8 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, 
>> uint32_t flags,
>>   Error **errp);
>>   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap 
>> *bitmap);
>>   void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
>> -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> - const char *name,
>> - Error **errp);
>> +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char 
>> *name,
>> +Error **errp);
>>   void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
>>   void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
>>   void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap);
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 8f42015db9..a52b83b619 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -455,13 +455,14 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState 
>> *bs)
>>* not fail.
>>* This function doesn't release corresponding BdrvDirtyBitmap.
>>*/
>> -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> - const char *name,
>> - Error **errp)
>> +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char 
>> *name,
>> +Error **errp)
>>   {
>>   if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
>> -bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
>> +return bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
>>   }
>> +
> 
> But is it a problem if we return an error code without setting errp now?
> If this is for the sake of not having to deal with local_err, we should
> make sure that a non-zero return means that errp is set. Right?

Oops, of course, I should set errp here

> 
>> +return -ENOTSUP;
>>   }
>>   
>>   bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char 
>> *name,
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index b2487101ed..1aaedb3b55 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1404,11 +1404,10 @@ static Qcow2Bitmap 
>> *find_bitmap_by_name(Qcow2BitmapList *bm_list,
>>   return NULL;
>>   }

Re: [Qemu-block] [PATCH 0/2] trace: Forbid trailing newline in event format

[Kevin Wolf]

Am 13.09.2019 um 12:52 hat Philippe Mathieu-Daudé geschrieben:
> Hi Stefan,
> 
> I'v been confused by trailing newline in trace reports,
> so this series aims to fix this, by cleaning current
> formats and add a check to catch new one introduced.

Good idea.

Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [PATCH v2] virtio-blk: schedule virtio_notify_config to run on main context

[Kevin Wolf]

Am 13.09.2019 um 12:56 hat Sergio Lopez geschrieben:
> virtio_notify_config() needs to acquire the global mutex, which isn't
> allowed from an iothread, and may lead to a deadlock like this:
> 
>  - main thead
>   * Has acquired: qemu_global_mutex.
>   * Is trying the acquire: iothread AioContext lock via
> AIO_WAIT_WHILE (after aio_poll).
> 
>  - iothread
>   * Has acquired: AioContext lock.
>   * Is trying to acquire: qemu_global_mutex (via
> virtio_notify_config->prepare_mmio_access).
> 
> If virtio_blk_resize() is called from an iothread, schedule
> virtio_notify_config() to be run in the main context BH.
> 
> Signed-off-by: Sergio Lopez 
> ---
> Changelog
> 
> v2:
>  - Use aio_bh_schedule_oneshot instead of scheduling a coroutine
>(thanks Kevin Wolf).
>  - Switch from RFC to v2 patch.
> ---
>  hw/block/virtio-blk.c | 21 -
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 18851601cb..669dc60f5b 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -16,6 +16,7 @@
>  #include "qemu/iov.h"
>  #include "qemu/module.h"
>  #include "qemu/error-report.h"
> +#include "qemu/main-loop.h"
>  #include "trace.h"
>  #include "hw/block/block.h"
>  #include "hw/qdev-properties.h"
> @@ -1086,11 +1087,29 @@ static int virtio_blk_load_device(VirtIODevice *vdev, 
> QEMUFile *f,
>  return 0;
>  }
>  
> +static void virtio_resize_cb(void *opaque)
> +{
> +VirtIODevice *vdev = opaque;
> +
> +assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> +virtio_notify_config(vdev);
> +}
> +
>  static void virtio_blk_resize(void *opaque)
>  {
>  VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
>  
> -virtio_notify_config(vdev);
> +if (qemu_get_current_aio_context() != qemu_get_aio_context()) {
> +/*
> + * virtio_notify_config() needs to acquire the global mutex,
> + * so it can't be called from an iothread. Instead, schedule
> + * it to be run in the main context BH.
> + */
> +aio_bh_schedule_oneshot(qemu_get_aio_context(),
> +virtio_resize_cb, vdev);
> +} else {
> +virtio_notify_config(vdev);

Let's call virtio_resize_cb() instead to keep both code paths the same.
Otherwise, we might add more code to virtio_resize_cb() later and miss
that it must be duplicated here.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v2] util/hbitmap: strict hbitmap_reset

[Kevin Wolf]

Am 13.09.2019 um 20:49 hat John Snow geschrieben:
> On 9/12/19 4:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> > Also, I'm not sure about "are" suggested by Max. "are" is for plural, but 
> > here I meant
> > one object: sum of @start and @count.
> > 
> 
> There's not great agreement universally about how to treat things like
> collective nouns. Sometimes "Data" is singular, but sometimes it's
> plural. "It depends."
> 
> In this case, "start + count" refers to one sum, but two constituent
> pieces, so it's functioning like a collective noun.
> 
> We might say "a + b (together) /are/ ..." but also "the sum of a + b /is/".
> 
> > So, you may use exactly "Sum of @start and @count is" or "(@start + @count) 
> > sum is" or
> > just "(@start + @count) is", whichever you like more.
> > 
> 
> I like using "the sum of @x and @y is" for being grammatically unambiguous.
> 
> updated and pushed.
> 
> (Sorry about my language again! --js)

Time to revive https://patchwork.kernel.org/patch/8725621/? ;-)

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH 3/4] iotests: Add @error to wait_until_completed

[Max Reitz]

On 14.09.19 00:53, John Snow wrote:
> 
> 
> On 9/12/19 9:56 AM, Max Reitz wrote:
>> Callers can use this new parameter to expect failure during the
>> completion process.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/iotests.py | 18 --
>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index b26271187c..300347c7c8 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -745,15 +745,20 @@ class QMPTestCase(unittest.TestCase):
>>  self.assert_no_active_block_jobs()
>>  return result
>>  
>> -def wait_until_completed(self, drive='drive0', check_offset=True, 
>> wait=60.0):
>> +def wait_until_completed(self, drive='drive0', check_offset=True, 
>> wait=60.0,
>> + error=None):
>>  '''Wait for a block job to finish, returning the event'''
>>  while True:
>>  for event in self.vm.get_qmp_events(wait=wait):
>>  if event['event'] == 'BLOCK_JOB_COMPLETED':
>>  self.assert_qmp(event, 'data/device', drive)
>> -self.assert_qmp_absent(event, 'data/error')
>> -if check_offset:
>> -self.assert_qmp(event, 'data/offset', 
>> event['data']['len'])
>> +if error is None:
>> +self.assert_qmp_absent(event, 'data/error')
>> +if check_offset:
>> +self.assert_qmp(event, 'data/offset',
>> +event['data']['len'])
>> +else:
>> +self.assert_qmp(event, 'data/error', error)
>>  self.assert_no_active_block_jobs()
>>  return event
>>  elif event['event'] == 'JOB_STATUS_CHANGE':
>> @@ -771,7 +776,8 @@ class QMPTestCase(unittest.TestCase):
>>  self.assert_qmp(event, 'data/type', 'mirror')
>>  self.assert_qmp(event, 'data/offset', event['data']['len'])
>>  
>> -def complete_and_wait(self, drive='drive0', wait_ready=True):
>> +def complete_and_wait(self, drive='drive0', wait_ready=True,
>> +  completion_error=None):
>>  '''Complete a block job and wait for it to finish'''
>>  if wait_ready:
>>  self.wait_ready(drive=drive)
>> @@ -779,7 +785,7 @@ class QMPTestCase(unittest.TestCase):
>>  result = self.vm.qmp('block-job-complete', device=drive)
>>  self.assert_qmp(result, 'return', {})
>>  
>> -event = self.wait_until_completed(drive=drive)
>> +event = self.wait_until_completed(drive=drive, 
>> error=completion_error)
>>  self.assert_qmp(event, 'data/type', 'mirror')
>>  
>>  def pause_wait(self, job_id='job0'):
>>
> 
> toot toot more optional parameters. lay them at the altar of
> noncommittal python design.
> 
> I completely forget what the difference between unittest.TestCase and
> qtest.QEMUQtestMachine is and why they each have job management methods.
> 
> Well, OK: the VM one is a simple subclass of the general-purpose VM
> machine to add some more useful stuff. the unittest one implements some
> general-purpose behavior with asserts that only work in the unittest world.

Yes.  run_job doesn’t assert anything (well, I could check the returned
error, but that’s it), it’s just based on comparison to the reference
output.  That’s not so useful for unittest-style tests, so it’s better
to use complete_and_wait() etc. there.

> Still,
> 
> It's a little fun that we've got run_job as well as cancel_and_wait,
> wait_until_completed, wait_ready, wait_ready_and_cancel, pause_wait and
> pause_job and they all seem to implement job run-state logic management
> a little differently.

At the end of the day, it’s all just test code.  It doesn’t hurt too
much for it to have duplicated code and be generally messy.

> Probably no bugs there, I bet.

Hm.  The functions are not that complex.

> *cough* Not your fault, anyway, so please take this accolade:
> 
> Reviewed-by: John Snow 

Thanks!

> (it's probably my fault)

I can only imagine by nonassistance of code in danger.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 1/2] blockdev: release the AioContext at drive_backup_prepare

[Kevin Wolf]

Am 13.09.2019 um 21:54 hat John Snow geschrieben:
> 
> 
> On 9/13/19 11:25 AM, Sergio Lopez wrote:
> > do_drive_backup() already acquires the AioContext, so release it
> > before the call.
> > 
> > Signed-off-by: Sergio Lopez 
> > ---
> >  blockdev.c | 6 +-
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index fbef6845c8..3927fdab80 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -1783,20 +1783,16 @@ static void drive_backup_prepare(BlkActionState 
> > *common, Error **errp)
> >  
> >  aio_context = bdrv_get_aio_context(bs);
> >  aio_context_acquire(aio_context);
> > -

Are you removing this unrelated empty line intentionally?

> >  /* Paired with .clean() */
> >  bdrv_drained_begin(bs);
> 
> Do we need to make this change to blockdev_backup_prepare as well?

Actually, the whole structure feels a bit wrong. We get the bs here and
take its lock, then release the lock again and forget the reference,
only to do both things again inside do_drive_backup().

The way snapshots work is that the "normal" snapshot commands are
wrappers that turn it into a single-entry transaction. Then you have
only one code path where you can resolve the ID and take the lock just
once. So maybe backup should work like this, too?

Kevin