mirror dead-lock

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 <[email protected]>

---
 block/mirror.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

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

Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>

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

Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>

---
 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

tests/qemu-iotests/235 new file +59 -0

new file mode 100755
index 0000000000..e5d1ae6593
@@ -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()

tests/qemu-iotests/235.out new file +1 -0

new file mode 100644
index 0000000000..d6ccbad8fa
@@ -0,0 +1 @@
+{u'return': {}}

tests/qemu-iotests/group +0 -0

index 8c56a0ad11..61a6d98ebd 100644

Quoted 10 lines · Vladimir Sementsov-Ogievskiy

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

Signed-off-by Vladimir Sementsov-Ogievskiy<[email protected]>

---
  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

235.diff

tests/qemu-iotests/235 +4 -5

index e5d1ae6593..c5264972b9 100755
@@ -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,

tests/qemu-iotests/235 +11 -0

+
+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()

tests/qemu-iotests/235.out +0 -0

index d6ccbad8fa..39db621e04 100644

Quoted 11 lines · Max Reitz

Quoted 10 lines · Vladimir Sementsov-Ogievskiy

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

Signed-off-by Vladimir Sementsov-Ogievskiy<[email protected]>

---
  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

Quoted 6 lines

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?

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

Quoted 19 lines · Vladimir Sementsov-Ogievskiy

Quoted 17 lines · Max Reitz

Quoted 10 lines · Vladimir Sementsov-Ogievskiy

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

Signed-off-by Vladimir Sementsov-Ogievskiy<[email protected]>

---
   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.

Quoted 1 line

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.

[...]

Quoted 1 line

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

Quoted 20 lines · Max Reitz

Quoted 19 lines · Vladimir Sementsov-Ogievskiy

Quoted 17 lines · Max Reitz

Quoted 10 lines · Vladimir Sementsov-Ogievskiy

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

Signed-off-by Vladimir Sementsov-Ogievskiy<[email protected]>

---
   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.

Quoted 5 lines

Quoted 1 line

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?

Am 30.11.2018 um 14:48 hat Vladimir Sementsov-Ogievskiy geschrieben:

Quoted 29 lines

Quoted 20 lines · Max Reitz

Quoted 19 lines · Vladimir Sementsov-Ogievskiy

Quoted 17 lines · Max Reitz

Quoted 10 lines · Vladimir Sementsov-Ogievskiy

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

Signed-off-by Vladimir Sementsov-Ogievskiy<[email protected]>

---
    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.

Quoted 5 lines

Quoted 1 line

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.

Quoted 3 lines

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

Quoted 34 lines · Kevin Wolf

Am 30.11.2018 um 14:48 hat Vladimir Sementsov-Ogievskiy geschrieben:

Quoted 32 lines

Quoted 20 lines · Max Reitz

Quoted 19 lines · Vladimir Sementsov-Ogievskiy

Quoted 17 lines · Max Reitz

Quoted 10 lines · Vladimir Sementsov-Ogievskiy

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

Signed-off-by Vladimir Sementsov-Ogievskiy<[email protected]>

---
    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.

Quoted 5 lines

Quoted 1 line

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?

Quoted 5 lines

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..

Quoted 38 lines · Vladimir Sementsov-Ogievskiy

Quoted 37 lines · Kevin Wolf

Am 30.11.2018 um 14:48 hat Vladimir Sementsov-Ogievskiy geschrieben:

Quoted 27 lines

Quoted 20 lines · Max Reitz

Quoted 19 lines · Vladimir Sementsov-Ogievskiy

Quoted 17 lines · Max Reitz

Quoted 10 lines · Vladimir Sementsov-Ogievskiy

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

Signed-off-by Vladimir Sementsov-Ogievskiy<[email protected]>

---
     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.

Quoted 5 lines

Quoted 1 line

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.

Quoted 5 lines

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?

Quoted 4 lines

Quoted 2 lines

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

Quoted 42 lines · Max Reitz

Quoted 41 lines · Vladimir Sementsov-Ogievskiy

Quoted 39 lines · Kevin Wolf

Am 30.11.2018 um 14:48 hat Vladimir Sementsov-Ogievskiy geschrieben:

Quoted 27 lines

Quoted 20 lines · Max Reitz

Quoted 19 lines · Vladimir Sementsov-Ogievskiy

Quoted 17 lines · Max Reitz

Quoted 10 lines · Vladimir Sementsov-Ogievskiy

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

Signed-off-by Vladimir Sementsov-Ogievskiy<[email protected]>

---
     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.

Quoted 5 lines

Quoted 1 line

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.

Quoted 5 lines

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

Quoted 5 lines

Quoted 1 line

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?

Quoted 49 lines · Vladimir Sementsov-Ogievskiy

Quoted 47 lines · Max Reitz

Quoted 35 lines · Vladimir Sementsov-Ogievskiy

Quoted 34 lines · Kevin Wolf

Am 30.11.2018 um 14:48 hat Vladimir Sementsov-Ogievskiy geschrieben:

Quoted 32 lines

Quoted 20 lines · Max Reitz

Quoted 19 lines · Vladimir Sementsov-Ogievskiy

Quoted 17 lines · Max Reitz

Quoted 10 lines · Vladimir Sementsov-Ogievskiy

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

Signed-off-by Vladimir Sementsov-Ogievskiy<[email protected]>

---
      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.

Quoted 5 lines

Quoted 1 line

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?

Quoted 7 lines

Quoted 5 lines

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.

Quoted 1 line

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

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

Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>

---
 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

tests/qemu-iotests/235.out new file +1 -0

new file mode 100644
index 0000000000..d6ccbad8fa
@@ -0,0 +1 @@
+{u'return': {}}

tests/qemu-iotests/group +0 -0

index 8c56a0ad11..61a6d98ebd 100644

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.

Quoted 6 lines · Vladimir Sementsov-Ogievskiy

Hi all!

v2: add fix:)

We've faced the following mirror bug:

Quoted 1 line

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?

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.

Quoted 7 lines · 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.

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

Quoted 18 lines · Max Reitz

Quoted 7 lines · 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.

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.

Quoted 2 lines

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.

Quoted 4 lines

Quoted 4 lines

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 :-)).

Quoted 4 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 1 line

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.

Quoted 4 lines · Vladimir Sementsov-Ogievskiy

Hi all!

v2: add fix:)

Quoted 3 lines

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 :-)).

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?