[Qemu-devel] [PATCH v2 0/2] mirror dead-lock

[Qemu-devel] [PATCH v2 0/2] mirror dead-lock

[Vladimir Sementsov-Ogievskiy]

Hi all!

v2: add fix:)

We've faced the following mirror bug:

Just run mirror on qcow2 image more than 1G, and qemu is in dead lock.

Dead lock described in 01, in short, we have extra aio_context_acquire
and aio_context_release around blk_aio_pwritev in mirror_read_complete.
So, write may yield to the main loop, and aio context is acquired. Main
loop than hangs on trying to lock BQL, which is locked by cpu thread,
and the cpu thread hangs on trying to acquire aio context.

Hm, now the thing looks fixed, by I still have a questions:

Is it a common thing, that we can't yield inside
aio_context_acquire/release ?

Was commit b9e413dd3756 
"block: explicitly acquire aiocontext in aio callbacks that need it"
wrong? Why it added these acquire/release, when it is written in 
multiple-iothreads.txt, that "Side note: the best way to schedule a function
call across threads is to call aio_bh_schedule_oneshot().  No acquire/release
or locking is needed."

Can someone in short describe, what BQL and aio context lock means, what they
protect, and haw they should cooperate?

Vladimir Sementsov-Ogievskiy (2):
  mirror: fix dead-lock
  iotests: simple mirror test with kvm on 1G image

 block/mirror.c             | 13 ++++-----
 tests/qemu-iotests/235     | 59 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/235.out |  1 +
 tests/qemu-iotests/group   |  1 +
 4 files changed, 66 insertions(+), 8 deletions(-)
 create mode 100755 tests/qemu-iotests/235
 create mode 100644 tests/qemu-iotests/235.out

-- 
2.18.0

[Qemu-devel] [PATCH v2 1/2] mirror: fix dead-lock

[Vladimir Sementsov-Ogievskiy]

Let start from the beginning:

Commit b9e413dd375 (in 2.9)
"block: explicitly acquire aiocontext in aio callbacks that need it"
added pairs of aio_context_acquire/release to mirror_write_complete and
mirror_read_complete, when they were aio callbacks for blk_aio_* calls.

Then, commit 2e1990b26e5 (in 3.0) "block/mirror: Convert to coroutines"
dropped these blk_aio_* calls, than mirror_write_complete and
mirror_read_complete are not callbacks more, and don't need additional
aiocontext acquiring. Furthermore, mirror_read_complete calls
blk_co_pwritev inside these pair of aio_context_acquire/release, which
leads to the following dead-lock with mirror:

 (gdb) info thr
   Id   Target Id         Frame
   3    Thread (LWP 145412) "qemu-system-x86" syscall ()
   2    Thread (LWP 145416) "qemu-system-x86" __lll_lock_wait ()
 * 1    Thread (LWP 145411) "qemu-system-x86" __lll_lock_wait ()

 (gdb) bt
 #0  __lll_lock_wait ()
 #1  _L_lock_812 ()
 #2  __GI___pthread_mutex_lock
 #3  qemu_mutex_lock_impl (mutex=0x561032dce420 <qemu_global_mutex>,
     file=0x5610327d8654 "util/main-loop.c", line=236) at
     util/qemu-thread-posix.c:66
 #4  qemu_mutex_lock_iothread_impl
 #5  os_host_main_loop_wait (timeout=480116000) at util/main-loop.c:236
 #6  main_loop_wait (nonblocking=0) at util/main-loop.c:497
 #7  main_loop () at vl.c:1892
 #8  main

Printing contents of qemu_global_mutex, I see that "__owner = 145416",
so, thr1 is main loop, and now it wants BQL, which is owned by thr2.

 (gdb) thr 2
 (gdb) bt
 #0  __lll_lock_wait ()
 #1  _L_lock_870 ()
 #2  __GI___pthread_mutex_lock
 #3  qemu_mutex_lock_impl (mutex=0x561034d25dc0, ...
 #4  aio_context_acquire (ctx=0x561034d25d60)
 #5  dma_blk_cb
 #6  dma_blk_io
 #7  dma_blk_read
 #8  ide_dma_cb
 #9  bmdma_cmd_writeb
 #10 bmdma_write
 #11 memory_region_write_accessor
 #12 access_with_adjusted_size
 #15 flatview_write
 #16 address_space_write
 #17 address_space_rw
 #18 kvm_handle_io
 #19 kvm_cpu_exec
 #20 qemu_kvm_cpu_thread_fn
 #21 qemu_thread_start
 #22 start_thread
 #23 clone ()

Printing mutex in fr 2, I see "__owner = 145411", so thr2 wants aio
context mutex, which is owned by thr1. Classic dead-lock.

Then, let's check that aio context is hold by mirror coroutine: just
print coroutine stack of first tracked request in mirror job target:

 (gdb) [...]
 (gdb) qemu coroutine 0x561035dd0860
 #0  qemu_coroutine_switch
 #1  qemu_coroutine_yield
 #2  qemu_co_mutex_lock_slowpath
 #3  qemu_co_mutex_lock
 #4  qcow2_co_pwritev
 #5  bdrv_driver_pwritev
 #6  bdrv_aligned_pwritev
 #7  bdrv_co_pwritev
 #8  blk_co_pwritev
 #9  mirror_read_complete () at block/mirror.c:232
 #10 mirror_co_read () at block/mirror.c:370
 #11 coroutine_trampoline
 #12 __start_context

Yes it is mirror_read_complete calling blk_co_pwritev after acquiring
aio context.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/mirror.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 56d9ef7474..8f52c6215d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -199,7 +199,6 @@ static void coroutine_fn mirror_write_complete(MirrorOp *op, int ret)
 {
     MirrorBlockJob *s = op->s;
 
-    aio_context_acquire(blk_get_aio_context(s->common.blk));
     if (ret < 0) {
         BlockErrorAction action;
 
@@ -209,15 +208,14 @@ static void coroutine_fn mirror_write_complete(MirrorOp *op, int ret)
             s->ret = ret;
         }
     }
+
     mirror_iteration_done(op, ret);
-    aio_context_release(blk_get_aio_context(s->common.blk));
 }
 
 static void coroutine_fn mirror_read_complete(MirrorOp *op, int ret)
 {
     MirrorBlockJob *s = op->s;
 
-    aio_context_acquire(blk_get_aio_context(s->common.blk));
     if (ret < 0) {
         BlockErrorAction action;
 
@@ -228,12 +226,11 @@ static void coroutine_fn mirror_read_complete(MirrorOp *op, int ret)
         }
 
         mirror_iteration_done(op, ret);
-    } else {
-        ret = blk_co_pwritev(s->target, op->offset,
-                             op->qiov.size, &op->qiov, 0);
-        mirror_write_complete(op, ret);
+        return;
     }
-    aio_context_release(blk_get_aio_context(s->common.blk));
+
+    ret = blk_co_pwritev(s->target, op->offset, op->qiov.size, &op->qiov, 0);
+    mirror_write_complete(op, ret);
 }
 
 /* Clip bytes relative to offset to not exceed end-of-file */
-- 
2.18.0

[Qemu-devel] [PATCH v2 2/2] iotests: simple mirror test with kvm on 1G image

[Vladimir Sementsov-Ogievskiy]

This test is broken without previous commit fixing dead-lock in mirror.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/235     | 59 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/235.out |  1 +
 tests/qemu-iotests/group   |  1 +
 3 files changed, 61 insertions(+)
 create mode 100755 tests/qemu-iotests/235
 create mode 100644 tests/qemu-iotests/235.out

diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
new file mode 100755
index 0000000000..e5d1ae6593
--- /dev/null
+++ b/tests/qemu-iotests/235
@@ -0,0 +1,59 @@
+#!/usr/bin/env python
+#
+# Simple mirror test
+#
+# Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
+#
+# 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 <http://www.gnu.org/licenses/>.
+#
+
+import sys
+import os
+import iotests
+from iotests import qemu_img_create, qemu_io, file_path
+
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts'))
+
+from qemu import QEMUMachine
+
+# Note:
+# This test was added to check that mirror dead-lock was fixed (see previous
+# commit before this test addition).
+# And it didn't reproduce if at least one of the following:
+# 1. use small image size
+# 2. use raw format (not qcow2)
+# 3. drop kvm and use iotests.VM() (maybe, because of qtest) (however, it still
+#    reproduces, if just drop kvm, but gdb failed to produce full backtraces
+#    for me)
+# 4. add iothread
+
+size = '1G'
+
+iotests.verify_image_format(supported_fmts=['raw', 'qcow2'])
+
+disk, target = file_path('disk', 'target')
+
+# prepare source image
+qemu_img_create('-f', iotests.imgfmt, disk, size)
+qemu_io('-f', iotests.imgfmt, '-c', 'write 0 ' + size, disk)
+
+vm = QEMUMachine(iotests.qemu_prog)
+vm.add_args('-machine', 'pc,accel=kvm')
+vm.add_args('-drive', 'id=src,file=' + disk)
+vm.launch()
+
+print(vm.qmp('drive-mirror', device='src', target=target, sync='full'))
+vm.event_wait('BLOCK_JOB_READY')
+
+vm.shutdown()
diff --git a/tests/qemu-iotests/235.out b/tests/qemu-iotests/235.out
new file mode 100644
index 0000000000..d6ccbad8fa
--- /dev/null
+++ b/tests/qemu-iotests/235.out
@@ -0,0 +1 @@
+{u'return': {}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 8c56a0ad11..61a6d98ebd 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -232,3 +232,4 @@
 232 auto quick
 233 auto quick
 234 auto quick migration
+235 auto quick
-- 
2.18.0

Re: [Qemu-devel] [Qemu-devel for-3.1?] [PATCH v2 0/2] mirror dead-lock

[Eric Blake]

On 11/29/18 4:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> v2: add fix:)
> 
> We've faced the following mirror bug:
> 
> Just run mirror on qcow2 image more than 1G, and qemu is in dead lock.

How long has the bug been present?  Based on patch 1 mentioning commit 
2e1990b26e5 (in 3.0), I'm guessing that this bug is not a regression 
specific to 3.1?  If so, then maybe we're better off deferring this to 
4.0 just relying on qemu-stable (already cc'd); on the other hand, if it 
is an easy-to-trigger deadlock on something the user is likely to do, 
can we justify it being severe enough to warrant -rc4?

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

Re: [Qemu-devel] [PATCH v2 2/2] iotests: simple mirror test with kvm on 1G image

[Kevin Wolf]

Am 29.11.2018 um 11:18 hat Vladimir Sementsov-Ogievskiy geschrieben:
> This test is broken without previous commit fixing dead-lock in mirror.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

--- /home/kwolf/source/qemu/tests/qemu-iotests/235.out  2018-11-30 12:57:11.275937057 +0100
+++ /home/kwolf/source/qemu/tests/qemu-iotests/235.out.bad      2018-11-30 13:17:33.890988974 +0100
@@ -1 +1 @@
-{u'return': {}}
+{'return': {}}
Failures: 235
Failed 1 of 1 tests


I think this is because you use print() (which produces output that is
inconsistent between Python 2 and 3) instead of iotests.log().

Kevin

Re: [Qemu-devel] [PATCH v2 2/2] iotests: simple mirror test with kvm on 1G image

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3913 bytes --]

On 29.11.18 11:18, Vladimir Sementsov-Ogievskiy wrote:
> This test is broken without previous commit fixing dead-lock in mirror.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/235     | 59 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/235.out |  1 +
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 61 insertions(+)
>  create mode 100755 tests/qemu-iotests/235
>  create mode 100644 tests/qemu-iotests/235.out

I'll get to the first patch in a second, but first a suggestion for this
patch: I think it's not so good to use 2 GB of space for a test (1 GB
for the source, 1 GB for the target).  So I tried my luck and found that
the test works, too, if you just use preallocation=metadata for the
source (instead of actually writing data) and blockdev-mirror'ing the
data to a throttled null-co device.

(Also, the test as-is passes even without patch 1 on tmpfs, at least for
me.  But with the configuration described above, it fails there, too.)

I've attached a diff.

Max

> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
> new file mode 100755
> index 0000000000..e5d1ae6593
> --- /dev/null
> +++ b/tests/qemu-iotests/235
> @@ -0,0 +1,59 @@
> +#!/usr/bin/env python
> +#
> +# Simple mirror test
> +#
> +# Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
> +#
> +# 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 <http://www.gnu.org/licenses/>.
> +#
> +
> +import sys
> +import os
> +import iotests
> +from iotests import qemu_img_create, qemu_io, file_path
> +
> +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts'))
> +
> +from qemu import QEMUMachine
> +
> +# Note:
> +# This test was added to check that mirror dead-lock was fixed (see previous
> +# commit before this test addition).
> +# And it didn't reproduce if at least one of the following:
> +# 1. use small image size
> +# 2. use raw format (not qcow2)
> +# 3. drop kvm and use iotests.VM() (maybe, because of qtest) (however, it still
> +#    reproduces, if just drop kvm, but gdb failed to produce full backtraces
> +#    for me)
> +# 4. add iothread
> +
> +size = '1G'
> +
> +iotests.verify_image_format(supported_fmts=['raw', 'qcow2'])
> +
> +disk, target = file_path('disk', 'target')
> +
> +# prepare source image
> +qemu_img_create('-f', iotests.imgfmt, disk, size)
> +qemu_io('-f', iotests.imgfmt, '-c', 'write 0 ' + size, disk)
> +
> +vm = QEMUMachine(iotests.qemu_prog)
> +vm.add_args('-machine', 'pc,accel=kvm')
> +vm.add_args('-drive', 'id=src,file=' + disk)
> +vm.launch()
> +
> +print(vm.qmp('drive-mirror', device='src', target=target, sync='full'))
> +vm.event_wait('BLOCK_JOB_READY')
> +
> +vm.shutdown()
> diff --git a/tests/qemu-iotests/235.out b/tests/qemu-iotests/235.out
> new file mode 100644
> index 0000000000..d6ccbad8fa
> --- /dev/null
> +++ b/tests/qemu-iotests/235.out
> @@ -0,0 +1 @@
> +{u'return': {}}
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 8c56a0ad11..61a6d98ebd 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -232,3 +232,4 @@
>  232 auto quick
>  233 auto quick
>  234 auto quick migration
> +235 auto quick
> 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 235.diff --]
[-- Type: text/x-patch; name="235.diff", Size: 1965 bytes --]

diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
index e5d1ae6593..c5264972b9 100755
--- a/tests/qemu-iotests/235
+++ b/tests/qemu-iotests/235
@@ -21,7 +21,7 @@
 import sys
 import os
 import iotests
-from iotests import qemu_img_create, qemu_io, file_path
+from iotests import qemu_img_create, qemu_io, file_path, log
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts'))
 
@@ -38,22 +38,34 @@ from qemu import QEMUMachine
 #    for me)
 # 4. add iothread
 
-size = '1G'
+size = 1 * 1024 * 1024 * 1024
 
 iotests.verify_image_format(supported_fmts=['raw', 'qcow2'])
 
-disk, target = file_path('disk', 'target')
+disk = file_path('disk')
 
 # prepare source image
-qemu_img_create('-f', iotests.imgfmt, disk, size)
-qemu_io('-f', iotests.imgfmt, '-c', 'write 0 ' + size, disk)
+qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
+                str(size))
 
 vm = QEMUMachine(iotests.qemu_prog)
 vm.add_args('-machine', 'pc,accel=kvm')
 vm.add_args('-drive', 'id=src,file=' + disk)
 vm.launch()
 
-print(vm.qmp('drive-mirror', device='src', target=target, sync='full'))
+log(vm.qmp('object-add', qom_type='throttle-group', id='tg0',
+           props={ 'x-bps-total': 64 * 1048576 }))
+
+log(vm.qmp('blockdev-add',
+           **{ 'node-name': 'target',
+               'driver': 'throttle',
+               'throttle-group': 'tg0',
+               'file': {
+                   'driver': 'null-co',
+                   'size': size
+                } }))
+
+log(vm.qmp('blockdev-mirror', device='src', target='target', sync='full'))
 vm.event_wait('BLOCK_JOB_READY')
 
 vm.shutdown()
diff --git a/tests/qemu-iotests/235.out b/tests/qemu-iotests/235.out
index d6ccbad8fa..39db621e04 100644
--- a/tests/qemu-iotests/235.out
+++ b/tests/qemu-iotests/235.out
@@ -1 +1,3 @@
-{u'return': {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] iotests: simple mirror test with kvm on 1G image

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]

On 29.11.18 11:18, Vladimir Sementsov-Ogievskiy wrote:
> This test is broken without previous commit fixing dead-lock in mirror.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/235     | 59 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/235.out |  1 +
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 61 insertions(+)
>  create mode 100755 tests/qemu-iotests/235
>  create mode 100644 tests/qemu-iotests/235.out

[...]

> diff --git a/tests/qemu-iotests/235.out b/tests/qemu-iotests/235.out
> new file mode 100644
> index 0000000000..d6ccbad8fa
> --- /dev/null
> +++ b/tests/qemu-iotests/235.out
> @@ -0,0 +1 @@
> +{u'return': {}}
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 8c56a0ad11..61a6d98ebd 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -232,3 +232,4 @@
>  232 auto quick
>  233 auto quick
>  234 auto quick migration
> +235 auto quick

Oh, and I don't really think this test belongs into the quick group.
Even if it is quick (depending on your hardware), it writes so much data
that I wouldn't put it there.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] mirror: fix dead-lock

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3395 bytes --]

On 29.11.18 11:18, Vladimir Sementsov-Ogievskiy wrote:
> Let start from the beginning:
> 
> Commit b9e413dd375 (in 2.9)
> "block: explicitly acquire aiocontext in aio callbacks that need it"
> added pairs of aio_context_acquire/release to mirror_write_complete and
> mirror_read_complete, when they were aio callbacks for blk_aio_* calls.
> 
> Then, commit 2e1990b26e5 (in 3.0) "block/mirror: Convert to coroutines"
> dropped these blk_aio_* calls, than mirror_write_complete and
> mirror_read_complete are not callbacks more, and don't need additional
> aiocontext acquiring.

Yes, that alone makes enough sense to me.

> Furthermore, mirror_read_complete calls
> blk_co_pwritev inside these pair of aio_context_acquire/release, which
> leads to the following dead-lock with mirror:
> 
>  (gdb) info thr
>    Id   Target Id         Frame
>    3    Thread (LWP 145412) "qemu-system-x86" syscall ()
>    2    Thread (LWP 145416) "qemu-system-x86" __lll_lock_wait ()
>  * 1    Thread (LWP 145411) "qemu-system-x86" __lll_lock_wait ()
> 
>  (gdb) bt
>  #0  __lll_lock_wait ()
>  #1  _L_lock_812 ()
>  #2  __GI___pthread_mutex_lock
>  #3  qemu_mutex_lock_impl (mutex=0x561032dce420 <qemu_global_mutex>,
>      file=0x5610327d8654 "util/main-loop.c", line=236) at
>      util/qemu-thread-posix.c:66
>  #4  qemu_mutex_lock_iothread_impl
>  #5  os_host_main_loop_wait (timeout=480116000) at util/main-loop.c:236
>  #6  main_loop_wait (nonblocking=0) at util/main-loop.c:497
>  #7  main_loop () at vl.c:1892
>  #8  main
> 
> Printing contents of qemu_global_mutex, I see that "__owner = 145416",
> so, thr1 is main loop, and now it wants BQL, which is owned by thr2.
> 
>  (gdb) thr 2
>  (gdb) bt
>  #0  __lll_lock_wait ()
>  #1  _L_lock_870 ()
>  #2  __GI___pthread_mutex_lock
>  #3  qemu_mutex_lock_impl (mutex=0x561034d25dc0, ...
>  #4  aio_context_acquire (ctx=0x561034d25d60)
>  #5  dma_blk_cb
>  #6  dma_blk_io
>  #7  dma_blk_read
>  #8  ide_dma_cb
>  #9  bmdma_cmd_writeb
>  #10 bmdma_write
>  #11 memory_region_write_accessor
>  #12 access_with_adjusted_size
>  #15 flatview_write
>  #16 address_space_write
>  #17 address_space_rw
>  #18 kvm_handle_io
>  #19 kvm_cpu_exec
>  #20 qemu_kvm_cpu_thread_fn
>  #21 qemu_thread_start
>  #22 start_thread
>  #23 clone ()
> 
> Printing mutex in fr 2, I see "__owner = 145411", so thr2 wants aio
> context mutex, which is owned by thr1. Classic dead-lock.
> 
> Then, let's check that aio context is hold by mirror coroutine: just
> print coroutine stack of first tracked request in mirror job target:
> 
>  (gdb) [...]
>  (gdb) qemu coroutine 0x561035dd0860
>  #0  qemu_coroutine_switch
>  #1  qemu_coroutine_yield
>  #2  qemu_co_mutex_lock_slowpath
>  #3  qemu_co_mutex_lock
>  #4  qcow2_co_pwritev
>  #5  bdrv_driver_pwritev
>  #6  bdrv_aligned_pwritev
>  #7  bdrv_co_pwritev
>  #8  blk_co_pwritev
>  #9  mirror_read_complete () at block/mirror.c:232
>  #10 mirror_co_read () at block/mirror.c:370
>  #11 coroutine_trampoline
>  #12 __start_context
> 
> Yes it is mirror_read_complete calling blk_co_pwritev after acquiring
> aio context.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/mirror.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-devel for-3.1?] [PATCH v2 0/2] mirror dead-lock

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 907 bytes --]

On 29.11.18 15:14, Eric Blake wrote:
> On 11/29/18 4:17 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> v2: add fix:)
>>
>> We've faced the following mirror bug:
>>
>> Just run mirror on qcow2 image more than 1G, and qemu is in dead lock.
> 
> How long has the bug been present?  Based on patch 1 mentioning commit
> 2e1990b26e5 (in 3.0), I'm guessing that this bug is not a regression
> specific to 3.1?  If so, then maybe we're better off deferring this to
> 4.0 just relying on qemu-stable (already cc'd); on the other hand, if it
> is an easy-to-trigger deadlock on something the user is likely to do,
> can we justify it being severe enough to warrant -rc4?

The test does indeed break on 3.0.0 (and passes on 2.12.0).  But mirror
just not working from non-trivial qcow2 images?  To me, that is a reason
to delay the release, even though it is not a regression.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] iotests: simple mirror test with kvm on 1G image

[Vladimir Sementsov-Ogievskiy]

30.11.2018 15:30, Max Reitz wrote:
> On 29.11.18 11:18, Vladimir Sementsov-Ogievskiy wrote:
>> This test is broken without previous commit fixing dead-lock in mirror.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/235     | 59 ++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/235.out |  1 +
>>   tests/qemu-iotests/group   |  1 +
>>   3 files changed, 61 insertions(+)
>>   create mode 100755 tests/qemu-iotests/235
>>   create mode 100644 tests/qemu-iotests/235.out
> I'll get to the first patch in a second, but first a suggestion for this
> patch: I think it's not so good to use 2 GB of space for a test (1 GB
> for the source, 1 GB for the target).  So I tried my luck and found that
> the test works, too, if you just use preallocation=metadata for the
> source (instead of actually writing data) and blockdev-mirror'ing the
> data to a throttled null-co device.

Hmm, so parsing metadata is enough for qcow2 to yield on write, yes?

> 
> (Also, the test as-is passes even without patch 1 on tmpfs, at least for
> me.  But with the configuration described above, it fails there, too.)
> 
> I've attached a diff.
> 
> Max
> 

[...]

>
> 
> 235.diff
> 
> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
> index e5d1ae6593..c5264972b9 100755
> --- a/tests/qemu-iotests/235
> +++ b/tests/qemu-iotests/235
> @@ -21,7 +21,7 @@
>   import sys
>   import os
>   import iotests
> -from iotests import qemu_img_create, qemu_io, file_path
> +from iotests import qemu_img_create, qemu_io, file_path, log
>   
>   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts'))
>   
> @@ -38,22 +38,34 @@ from qemu import QEMUMachine
>   #    for me)
>   # 4. add iothread
>   
> -size = '1G'
> +size = 1 * 1024 * 1024 * 1024
>   
>   iotests.verify_image_format(supported_fmts=['raw', 'qcow2'])
>   
> -disk, target = file_path('disk', 'target')
> +disk = file_path('disk')
>   
>   # prepare source image
> -qemu_img_create('-f', iotests.imgfmt, disk, size)
> -qemu_io('-f', iotests.imgfmt, '-c', 'write 0 ' + size, disk)
> +qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
> +                str(size))
>   
>   vm = QEMUMachine(iotests.qemu_prog)
>   vm.add_args('-machine', 'pc,accel=kvm')
>   vm.add_args('-drive', 'id=src,file=' + disk)
>   vm.launch()
>   
> -print(vm.qmp('drive-mirror', device='src', target=target, sync='full'))
> +log(vm.qmp('object-add', qom_type='throttle-group', id='tg0',
> +           props={ 'x-bps-total': 64 * 1048576 }))

and you add throttling. interesting, isn't enough to mirror empty qcow2(no preallocation)
or even zero driver (without a qcow2 at all) to null with throttling? Or, otherwise, why we need throttling here?

> +
> +log(vm.qmp('blockdev-add',
> +           **{ 'node-name': 'target',
> +               'driver': 'throttle',
> +               'throttle-group': 'tg0',
> +               'file': {
> +                   'driver': 'null-co',
> +                   'size': size
> +                } }))
> +
> +log(vm.qmp('blockdev-mirror', device='src', target='target', sync='full'))
>   vm.event_wait('BLOCK_JOB_READY')
>   
>   vm.shutdown()
> diff --git a/tests/qemu-iotests/235.out b/tests/qemu-iotests/235.out
> index d6ccbad8fa..39db621e04 100644
> --- a/tests/qemu-iotests/235.out
> +++ b/tests/qemu-iotests/235.out
> @@ -1 +1,3 @@
> -{u'return': {}}
> +{"return": {}}
> +{"return": {}}
> +{"return": {}}
> 


anyway, it's good thing to avoid 2G allocation in test, I agree.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 2/2] iotests: simple mirror test with kvm on 1G image

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1595 bytes --]

On 30.11.18 14:06, Vladimir Sementsov-Ogievskiy wrote:
> 30.11.2018 15:30, Max Reitz wrote:
>> On 29.11.18 11:18, Vladimir Sementsov-Ogievskiy wrote:
>>> This test is broken without previous commit fixing dead-lock in mirror.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>>> ---
>>>   tests/qemu-iotests/235     | 59 ++++++++++++++++++++++++++++++++++++++
>>>   tests/qemu-iotests/235.out |  1 +
>>>   tests/qemu-iotests/group   |  1 +
>>>   3 files changed, 61 insertions(+)
>>>   create mode 100755 tests/qemu-iotests/235
>>>   create mode 100644 tests/qemu-iotests/235.out
>> I'll get to the first patch in a second, but first a suggestion for this
>> patch: I think it's not so good to use 2 GB of space for a test (1 GB
>> for the source, 1 GB for the target).  So I tried my luck and found that
>> the test works, too, if you just use preallocation=metadata for the
>> source (instead of actually writing data) and blockdev-mirror'ing the
>> data to a throttled null-co device.
> 
> Hmm, so parsing metadata is enough for qcow2 to yield on write, yes?

Apparently so.  If you can confirm that applying those changes to the
test still make it work (i.e., fail before patch 1, pass afterwards),
then I think it is just as good.

[...]

> anyway, it's good thing to avoid 2G allocation in test, I agree.

And if for some reason the test needs to keep doing that, I'd propose
adding some new test group for tests that do use such a large amount of
disk space so it can be excluded if people don't want that to happen.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] iotests: simple mirror test with kvm on 1G image

[Vladimir Sementsov-Ogievskiy]

30.11.2018 16:13, Max Reitz wrote:
> On 30.11.18 14:06, Vladimir Sementsov-Ogievskiy wrote:
>> 30.11.2018 15:30, Max Reitz wrote:
>>> On 29.11.18 11:18, Vladimir Sementsov-Ogievskiy wrote:
>>>> This test is broken without previous commit fixing dead-lock in mirror.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>>>> ---
>>>>    tests/qemu-iotests/235     | 59 ++++++++++++++++++++++++++++++++++++++
>>>>    tests/qemu-iotests/235.out |  1 +
>>>>    tests/qemu-iotests/group   |  1 +
>>>>    3 files changed, 61 insertions(+)
>>>>    create mode 100755 tests/qemu-iotests/235
>>>>    create mode 100644 tests/qemu-iotests/235.out
>>> I'll get to the first patch in a second, but first a suggestion for this
>>> patch: I think it's not so good to use 2 GB of space for a test (1 GB
>>> for the source, 1 GB for the target).  So I tried my luck and found that
>>> the test works, too, if you just use preallocation=metadata for the
>>> source (instead of actually writing data) and blockdev-mirror'ing the
>>> data to a throttled null-co device.
>>
>> Hmm, so parsing metadata is enough for qcow2 to yield on write, yes?
> 
> Apparently so.  If you can confirm that applying those changes to the
> test still make it work (i.e., fail before patch 1, pass afterwards),
> then I think it is just as good.

Ok, I've checked that your changes works for me.

hm, but we write to null, so, yield on write comes from throttling, however,
without preallocation=metadata, it don't work.., do you know, why we need
preallocation to reproduce?


> 
> [...]
> 
>> anyway, it's good thing to avoid 2G allocation in test, I agree.
> 
> And if for some reason the test needs to keep doing that, I'd propose
> adding some new test group for tests that do use such a large amount of
> disk space so it can be excluded if people don't want that to happen.
> 
> Max
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 2/2] iotests: simple mirror test with kvm on 1G image

[Kevin Wolf]

Am 30.11.2018 um 14:48 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 30.11.2018 16:13, Max Reitz wrote:
> > On 30.11.18 14:06, Vladimir Sementsov-Ogievskiy wrote:
> >> 30.11.2018 15:30, Max Reitz wrote:
> >>> On 29.11.18 11:18, Vladimir Sementsov-Ogievskiy wrote:
> >>>> This test is broken without previous commit fixing dead-lock in mirror.
> >>>>
> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> >>>> ---
> >>>>    tests/qemu-iotests/235     | 59 ++++++++++++++++++++++++++++++++++++++
> >>>>    tests/qemu-iotests/235.out |  1 +
> >>>>    tests/qemu-iotests/group   |  1 +
> >>>>    3 files changed, 61 insertions(+)
> >>>>    create mode 100755 tests/qemu-iotests/235
> >>>>    create mode 100644 tests/qemu-iotests/235.out
> >>> I'll get to the first patch in a second, but first a suggestion for this
> >>> patch: I think it's not so good to use 2 GB of space for a test (1 GB
> >>> for the source, 1 GB for the target).  So I tried my luck and found that
> >>> the test works, too, if you just use preallocation=metadata for the
> >>> source (instead of actually writing data) and blockdev-mirror'ing the
> >>> data to a throttled null-co device.
> >>
> >> Hmm, so parsing metadata is enough for qcow2 to yield on write, yes?
> > 
> > Apparently so.  If you can confirm that applying those changes to the
> > test still make it work (i.e., fail before patch 1, pass afterwards),
> > then I think it is just as good.
> 
> Ok, I've checked that your changes works for me.
> 
> hm, but we write to null, so, yield on write comes from throttling, however,
> without preallocation=metadata, it don't work.., do you know, why we need
> preallocation to reproduce?

If I should take a guess, probably because mirror only copies allocated
clusters?

Kevin

Re: [Qemu-devel] [PATCH v2 2/2] iotests: simple mirror test with kvm on 1G image

[Vladimir Sementsov-Ogievskiy]

30.11.2018 17:10, Kevin Wolf wrote:
> Am 30.11.2018 um 14:48 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 30.11.2018 16:13, Max Reitz wrote:
>>> On 30.11.18 14:06, Vladimir Sementsov-Ogievskiy wrote:
>>>> 30.11.2018 15:30, Max Reitz wrote:
>>>>> On 29.11.18 11:18, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> This test is broken without previous commit fixing dead-lock in mirror.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>>>>>> ---
>>>>>>     tests/qemu-iotests/235     | 59 ++++++++++++++++++++++++++++++++++++++
>>>>>>     tests/qemu-iotests/235.out |  1 +
>>>>>>     tests/qemu-iotests/group   |  1 +
>>>>>>     3 files changed, 61 insertions(+)
>>>>>>     create mode 100755 tests/qemu-iotests/235
>>>>>>     create mode 100644 tests/qemu-iotests/235.out
>>>>> I'll get to the first patch in a second, but first a suggestion for this
>>>>> patch: I think it's not so good to use 2 GB of space for a test (1 GB
>>>>> for the source, 1 GB for the target).  So I tried my luck and found that
>>>>> the test works, too, if you just use preallocation=metadata for the
>>>>> source (instead of actually writing data) and blockdev-mirror'ing the
>>>>> data to a throttled null-co device.
>>>>
>>>> Hmm, so parsing metadata is enough for qcow2 to yield on write, yes?
>>>
>>> Apparently so.  If you can confirm that applying those changes to the
>>> test still make it work (i.e., fail before patch 1, pass afterwards),
>>> then I think it is just as good.
>>
>> Ok, I've checked that your changes works for me.
>>
>> hm, but we write to null, so, yield on write comes from throttling, however,
>> without preallocation=metadata, it don't work.., do you know, why we need
>> preallocation to reproduce?
> 
> If I should take a guess, probably because mirror only copies allocated
> clusters?
> 
> Kevin
> 

hm, so, what preallocation=metadata does? I thought, it preallocates tables, but all qcow2 clusters would be unallocated..


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 2/2] iotests: simple mirror test with kvm on 1G image

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2515 bytes --]

On 30.11.18 15:51, Vladimir Sementsov-Ogievskiy wrote:
> 30.11.2018 17:10, Kevin Wolf wrote:
>> Am 30.11.2018 um 14:48 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>> 30.11.2018 16:13, Max Reitz wrote:
>>>> On 30.11.18 14:06, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 30.11.2018 15:30, Max Reitz wrote:
>>>>>> On 29.11.18 11:18, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>> This test is broken without previous commit fixing dead-lock in mirror.
>>>>>>>
>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>>>>>>> ---
>>>>>>>     tests/qemu-iotests/235     | 59 ++++++++++++++++++++++++++++++++++++++
>>>>>>>     tests/qemu-iotests/235.out |  1 +
>>>>>>>     tests/qemu-iotests/group   |  1 +
>>>>>>>     3 files changed, 61 insertions(+)
>>>>>>>     create mode 100755 tests/qemu-iotests/235
>>>>>>>     create mode 100644 tests/qemu-iotests/235.out
>>>>>> I'll get to the first patch in a second, but first a suggestion for this
>>>>>> patch: I think it's not so good to use 2 GB of space for a test (1 GB
>>>>>> for the source, 1 GB for the target).  So I tried my luck and found that
>>>>>> the test works, too, if you just use preallocation=metadata for the
>>>>>> source (instead of actually writing data) and blockdev-mirror'ing the
>>>>>> data to a throttled null-co device.
>>>>>
>>>>> Hmm, so parsing metadata is enough for qcow2 to yield on write, yes?
>>>>
>>>> Apparently so.  If you can confirm that applying those changes to the
>>>> test still make it work (i.e., fail before patch 1, pass afterwards),
>>>> then I think it is just as good.
>>>
>>> Ok, I've checked that your changes works for me.
>>>
>>> hm, but we write to null, so, yield on write comes from throttling, however,
>>> without preallocation=metadata, it don't work.., do you know, why we need
>>> preallocation to reproduce?
>>
>> If I should take a guess, probably because mirror only copies allocated
>> clusters?
>>
>> Kevin
>>
> 
> hm, so, what preallocation=metadata does? I thought, it preallocates tables, but all qcow2 clusters would be unallocated..

No, it initializes all metadata structures (i.e., L2 tables) so they
point to data clusters; which is why you cannot use it with backing
files.  It's just that the data clusters are not written.

It might make sense to use preallocated zero clusters in qcow2 v3; but
then writing to the image would cause COWs again, so there wouldn't be
too much benefit over just not preallocating.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] iotests: simple mirror test with kvm on 1G image

[Vladimir Sementsov-Ogievskiy]

30.11.2018 17:59, Max Reitz wrote:
> On 30.11.18 15:51, Vladimir Sementsov-Ogievskiy wrote:
>> 30.11.2018 17:10, Kevin Wolf wrote:
>>> Am 30.11.2018 um 14:48 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> 30.11.2018 16:13, Max Reitz wrote:
>>>>> On 30.11.18 14:06, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> 30.11.2018 15:30, Max Reitz wrote:
>>>>>>> On 29.11.18 11:18, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>> This test is broken without previous commit fixing dead-lock in mirror.
>>>>>>>>
>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>>>>>>>> ---
>>>>>>>>      tests/qemu-iotests/235     | 59 ++++++++++++++++++++++++++++++++++++++
>>>>>>>>      tests/qemu-iotests/235.out |  1 +
>>>>>>>>      tests/qemu-iotests/group   |  1 +
>>>>>>>>      3 files changed, 61 insertions(+)
>>>>>>>>      create mode 100755 tests/qemu-iotests/235
>>>>>>>>      create mode 100644 tests/qemu-iotests/235.out
>>>>>>> I'll get to the first patch in a second, but first a suggestion for this
>>>>>>> patch: I think it's not so good to use 2 GB of space for a test (1 GB
>>>>>>> for the source, 1 GB for the target).  So I tried my luck and found that
>>>>>>> the test works, too, if you just use preallocation=metadata for the
>>>>>>> source (instead of actually writing data) and blockdev-mirror'ing the
>>>>>>> data to a throttled null-co device.
>>>>>>
>>>>>> Hmm, so parsing metadata is enough for qcow2 to yield on write, yes?
>>>>>
>>>>> Apparently so.  If you can confirm that applying those changes to the
>>>>> test still make it work (i.e., fail before patch 1, pass afterwards),
>>>>> then I think it is just as good.
>>>>
>>>> Ok, I've checked that your changes works for me.
>>>>
>>>> hm, but we write to null, so, yield on write comes from throttling, however,
>>>> without preallocation=metadata, it don't work.., do you know, why we need
>>>> preallocation to reproduce?
>>>
>>> If I should take a guess, probably because mirror only copies allocated
>>> clusters?
>>>
>>> Kevin
>>>
>>
>> hm, so, what preallocation=metadata does? I thought, it preallocates tables, but all qcow2 clusters would be unallocated..
> 
> No, it initializes all metadata structures (i.e., L2 tables) so they
> point to data clusters; which is why you cannot use it with backing
> files.  It's just that the data clusters are not written.

so, clusters are allocated? in this case, we don't avoid allocating of 1G, but only have a performance gain?

> 
> It might make sense to use preallocated zero clusters in qcow2 v3; but
> then writing to the image would cause COWs again, so there wouldn't be
> too much benefit over just not preallocating.
> 
> Max
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 2/2] iotests: simple mirror test with kvm on 1G image

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2820 bytes --]

On 30.11.18 16:24, Vladimir Sementsov-Ogievskiy wrote:
> 30.11.2018 17:59, Max Reitz wrote:
>> On 30.11.18 15:51, Vladimir Sementsov-Ogievskiy wrote:
>>> 30.11.2018 17:10, Kevin Wolf wrote:
>>>> Am 30.11.2018 um 14:48 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>> 30.11.2018 16:13, Max Reitz wrote:
>>>>>> On 30.11.18 14:06, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>> 30.11.2018 15:30, Max Reitz wrote:
>>>>>>>> On 29.11.18 11:18, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>>> This test is broken without previous commit fixing dead-lock in mirror.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>>>>>>>>> ---
>>>>>>>>>      tests/qemu-iotests/235     | 59 ++++++++++++++++++++++++++++++++++++++
>>>>>>>>>      tests/qemu-iotests/235.out |  1 +
>>>>>>>>>      tests/qemu-iotests/group   |  1 +
>>>>>>>>>      3 files changed, 61 insertions(+)
>>>>>>>>>      create mode 100755 tests/qemu-iotests/235
>>>>>>>>>      create mode 100644 tests/qemu-iotests/235.out
>>>>>>>> I'll get to the first patch in a second, but first a suggestion for this
>>>>>>>> patch: I think it's not so good to use 2 GB of space for a test (1 GB
>>>>>>>> for the source, 1 GB for the target).  So I tried my luck and found that
>>>>>>>> the test works, too, if you just use preallocation=metadata for the
>>>>>>>> source (instead of actually writing data) and blockdev-mirror'ing the
>>>>>>>> data to a throttled null-co device.
>>>>>>>
>>>>>>> Hmm, so parsing metadata is enough for qcow2 to yield on write, yes?
>>>>>>
>>>>>> Apparently so.  If you can confirm that applying those changes to the
>>>>>> test still make it work (i.e., fail before patch 1, pass afterwards),
>>>>>> then I think it is just as good.
>>>>>
>>>>> Ok, I've checked that your changes works for me.
>>>>>
>>>>> hm, but we write to null, so, yield on write comes from throttling, however,
>>>>> without preallocation=metadata, it don't work.., do you know, why we need
>>>>> preallocation to reproduce?
>>>>
>>>> If I should take a guess, probably because mirror only copies allocated
>>>> clusters?
>>>>
>>>> Kevin
>>>>
>>>
>>> hm, so, what preallocation=metadata does? I thought, it preallocates tables, but all qcow2 clusters would be unallocated..
>>
>> No, it initializes all metadata structures (i.e., L2 tables) so they
>> point to data clusters; which is why you cannot use it with backing
>> files.  It's just that the data clusters are not written.
> 
> so, clusters are allocated? in this case, we don't avoid allocating of 1G, but only have a performance gain?

The data isn't written, so less than 1 MB are actually allocated on-disk.

The L2 tables do point to offsets over a range of 1 GB, but they should
all be holes on the filesystem.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] iotests: simple mirror test with kvm on 1G image

[Vladimir Sementsov-Ogievskiy]

30.11.2018 18:33, Max Reitz wrote:
> On 30.11.18 16:24, Vladimir Sementsov-Ogievskiy wrote:
>> 30.11.2018 17:59, Max Reitz wrote:
>>> On 30.11.18 15:51, Vladimir Sementsov-Ogievskiy wrote:
>>>> 30.11.2018 17:10, Kevin Wolf wrote:
>>>>> Am 30.11.2018 um 14:48 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>>> 30.11.2018 16:13, Max Reitz wrote:
>>>>>>> On 30.11.18 14:06, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>> 30.11.2018 15:30, Max Reitz wrote:
>>>>>>>>> On 29.11.18 11:18, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>>>> This test is broken without previous commit fixing dead-lock in mirror.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>>>>>>>>>> ---
>>>>>>>>>>       tests/qemu-iotests/235     | 59 ++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>       tests/qemu-iotests/235.out |  1 +
>>>>>>>>>>       tests/qemu-iotests/group   |  1 +
>>>>>>>>>>       3 files changed, 61 insertions(+)
>>>>>>>>>>       create mode 100755 tests/qemu-iotests/235
>>>>>>>>>>       create mode 100644 tests/qemu-iotests/235.out
>>>>>>>>> I'll get to the first patch in a second, but first a suggestion for this
>>>>>>>>> patch: I think it's not so good to use 2 GB of space for a test (1 GB
>>>>>>>>> for the source, 1 GB for the target).  So I tried my luck and found that
>>>>>>>>> the test works, too, if you just use preallocation=metadata for the
>>>>>>>>> source (instead of actually writing data) and blockdev-mirror'ing the
>>>>>>>>> data to a throttled null-co device.
>>>>>>>>
>>>>>>>> Hmm, so parsing metadata is enough for qcow2 to yield on write, yes?
>>>>>>>
>>>>>>> Apparently so.  If you can confirm that applying those changes to the
>>>>>>> test still make it work (i.e., fail before patch 1, pass afterwards),
>>>>>>> then I think it is just as good.
>>>>>>
>>>>>> Ok, I've checked that your changes works for me.
>>>>>>
>>>>>> hm, but we write to null, so, yield on write comes from throttling, however,
>>>>>> without preallocation=metadata, it don't work.., do you know, why we need
>>>>>> preallocation to reproduce?
>>>>>
>>>>> If I should take a guess, probably because mirror only copies allocated
>>>>> clusters?
>>>>>
>>>>> Kevin
>>>>>
>>>>
>>>> hm, so, what preallocation=metadata does? I thought, it preallocates tables, but all qcow2 clusters would be unallocated..
>>>
>>> No, it initializes all metadata structures (i.e., L2 tables) so they
>>> point to data clusters; which is why you cannot use it with backing
>>> files.  It's just that the data clusters are not written.
>>
>> so, clusters are allocated? in this case, we don't avoid allocating of 1G, but only have a performance gain?
> 
> The data isn't written, so less than 1 MB are actually allocated on-disk.
> 
> The L2 tables do point to offsets over a range of 1 GB, but they should
> all be holes on the filesystem.
> 
> Max
> 

Understand, ok, thank you!


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 0/2] mirror dead-lock

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 672 bytes --]

On 29.11.18 11:17, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> v2: add fix:)
> 
> We've faced the following mirror bug:
> 
> Just run mirror on qcow2 image more than 1G, and qemu is in dead lock.

So because apparently there is going to be an rc4 anyway (like basically
always...), I'd really like to bring this fix into it, unless there are
any objections from anyone (though all of you are more than welcome to
explicitly agree, too :-)).

Do you have any plans for the iotest?  Right now, I'd rather just take
patch 1 as-is and add the test later, but then again, adding a patch for
rc4 without a test is not so nice either, I suppose.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 0/2] mirror dead-lock

[Vladimir Sementsov-Ogievskiy]

03.12.2018 16:59, Max Reitz wrote:
> On 29.11.18 11:17, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> v2: add fix:)
>>
>> We've faced the following mirror bug:
>>
>> Just run mirror on qcow2 image more than 1G, and qemu is in dead lock.
> 
> So because apparently there is going to be an rc4 anyway (like basically
> always...), I'd really like to bring this fix into it, unless there are
> any objections from anyone (though all of you are more than welcome to
> explicitly agree, too :-)).
> 
> Do you have any plans for the iotest?  Right now, I'd rather just take
> patch 1 as-is and add the test later, but then again, adding a patch for
> rc4 without a test is not so nice either, I suppose.
> 
> Max
> 

I think, everything is better with test:) I can't say that I really like your
additions, because it's anyway a kind of cheating, less real-life, but on the
other hand, as I understand, allocating a lot of disk space in iotests is a bad
thing too.

May be it should be a kind of parameter, with default to your variant, something
like ./check --big-disk-allocations-allowed :). But let's commit at least the
test with your additions.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 0/2] mirror dead-lock

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1884 bytes --]

On 03.12.18 15:13, Vladimir Sementsov-Ogievskiy wrote:
> 03.12.2018 16:59, Max Reitz wrote:
>> On 29.11.18 11:17, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> v2: add fix:)
>>>
>>> We've faced the following mirror bug:
>>>
>>> Just run mirror on qcow2 image more than 1G, and qemu is in dead lock.
>>
>> So because apparently there is going to be an rc4 anyway (like basically
>> always...), I'd really like to bring this fix into it, unless there are
>> any objections from anyone (though all of you are more than welcome to
>> explicitly agree, too :-)).
>>
>> Do you have any plans for the iotest?  Right now, I'd rather just take
>> patch 1 as-is and add the test later, but then again, adding a patch for
>> rc4 without a test is not so nice either, I suppose.
>>
>> Max
>>
> 
> I think, everything is better with test:) I can't say that I really like your
> additions, because it's anyway a kind of cheating, less real-life,

You mean like all iotests? ;-)

iotests usually only test a single specific thing and not a full
real-life case.

>                                                                    but on the
> other hand, as I understand, allocating a lot of disk space in iotests is a bad
> thing too.
> 
> May be it should be a kind of parameter, with default to your variant, something
> like ./check --big-disk-allocations-allowed :).

As I said, we would need to add a new group (e.g. "big-disk-allocation")
and then probably disable that group in check by default.  You could run
those tests with ./check -g big-disk-allocation.

> But let's commit at least the test with your additions.

I mean, we can also add both tests.  But I should say that your version
did not fail on tmpfs before this fix, and I usually run tests on tmpfs,
so...  It wouldn't be very indicative of the issue for me.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 0/2] mirror dead-lock

[Peter Maydell]

On Mon, 3 Dec 2018 at 14:03, Max Reitz <mreitz@redhat.com> wrote:
>
> On 29.11.18 11:17, Vladimir Sementsov-Ogievskiy wrote:
> > Hi all!
> >
> > v2: add fix:)
> >
> > We've faced the following mirror bug:
> >
> > Just run mirror on qcow2 image more than 1G, and qemu is in dead lock.
>
> So because apparently there is going to be an rc4 anyway (like basically
> always...), I'd really like to bring this fix into it, unless there are
> any objections from anyone (though all of you are more than welcome to
> explicitly agree, too :-)).

Discussion on IRC seemed to lean in favour of putting this fix into
rc4 as well. Could somebody (not sure if this would be you or Kevin)
send a pullrequest with it in today, please ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 0/2] mirror dead-lock

[Kevin Wolf]

Am 29.11.2018 um 11:17 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi all!
> 
> v2: add fix:)
> 
> We've faced the following mirror bug:
> 
> Just run mirror on qcow2 image more than 1G, and qemu is in dead lock.
> 
> Dead lock described in 01, in short, we have extra aio_context_acquire
> and aio_context_release around blk_aio_pwritev in mirror_read_complete.
> So, write may yield to the main loop, and aio context is acquired. Main
> loop than hangs on trying to lock BQL, which is locked by cpu thread,
> and the cpu thread hangs on trying to acquire aio context.
> 
> Hm, now the thing looks fixed, by I still have a questions:
> 
> Is it a common thing, that we can't yield inside
> aio_context_acquire/release ?
> 
> Was commit b9e413dd3756 
> "block: explicitly acquire aiocontext in aio callbacks that need it"
> wrong? Why it added these acquire/release, when it is written in 
> multiple-iothreads.txt, that "Side note: the best way to schedule a function
> call across threads is to call aio_bh_schedule_oneshot().  No acquire/release
> or locking is needed."
> 
> Can someone in short describe, what BQL and aio context lock means, what they
> protect, and haw they should cooperate?

Thanks, applied patch 1 to the block branch. (We'll use Max' v3 for the
test case.)

Kevin

Re: [Qemu-devel] [PATCH v2 0/2] mirror dead-lock

[Eric Blake]

On 12/3/18 8:26 AM, Max Reitz wrote:

>>> So because apparently there is going to be an rc4 anyway (like basically
>>> always...), I'd really like to bring this fix into it, unless there are
>>> any objections from anyone (though all of you are more than welcome to
>>> explicitly agree, too :-)).

I agree with fixing this in -rc4.

>>>
>>> Do you have any plans for the iotest?  Right now, I'd rather just take
>>> patch 1 as-is and add the test later, but then again, adding a patch for
>>> rc4 without a test is not so nice either, I suppose.

...


>>
>> May be it should be a kind of parameter, with default to your variant, something
>> like ./check --big-disk-allocations-allowed :).
> 
> As I said, we would need to add a new group (e.g. "big-disk-allocation")
> and then probably disable that group in check by default.  You could run
> those tests with ./check -g big-disk-allocation.
> 
>> But let's commit at least the test with your additions.
> 
> I mean, we can also add both tests.  But I should say that your version
> did not fail on tmpfs before this fix, and I usually run tests on tmpfs,
> so...  It wouldn't be very indicative of the issue for me.

My take - patch 1 for fixing the bug, plus Max's patch 3 for quickly 
testing the bug for 3.1. Vladimir's patch 2 should defer to 4.0, if we 
want it at all, since we're still debating about whether we need it (and 
even how we would spell it in iotests to run or not run it under 
particular setups).

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