[Qemu-devel] [PULL 0/2] Block layer patches

[Qemu-devel] [PULL 0/2] Block layer patches

[Kevin Wolf]

The following changes since commit 83ea23cd207a03c5736be0231acbf7f8b05dbf52:

  i386: hvf: Fix overrun of _decode_tbl1 (2018-12-03 15:09:55 +0000)

are available in the Git repository at:

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

for you to fetch changes up to db5e8210adbafe9c6383d8364345a8c545d38e62:

  iotests: simple mirror test with kvm on 1G image (2018-12-03 16:51:53 +0100)

----------------------------------------------------------------
Block layer patches:

- mirror: Fix deadlock

----------------------------------------------------------------
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     | 76 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/235.out |  3 ++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 85 insertions(+), 8 deletions(-)
 create mode 100755 tests/qemu-iotests/235
 create mode 100644 tests/qemu-iotests/235.out

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

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.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.19.2

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

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
Acked-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/235     | 76 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/235.out |  3 ++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 80 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..da044ed34e
--- /dev/null
+++ b/tests/qemu-iotests/235
@@ -0,0 +1,76 @@
+#!/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, log
+
+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 = 1 * 1024 * 1024 * 1024
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+
+disk = file_path('disk')
+
+# prepare source image
+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()
+
+log(vm.qmp('object-add', qom_type='throttle-group', id='tg0',
+           props={ 'x-bps-total': size }))
+
+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'))
+
+try:
+    vm.event_wait('BLOCK_JOB_READY', timeout=10.0)
+except:
+    vm.shutdown()
+    raise
+
+vm.shutdown()
diff --git a/tests/qemu-iotests/235.out b/tests/qemu-iotests/235.out
new file mode 100644
index 0000000000..39db621e04
--- /dev/null
+++ b/tests/qemu-iotests/235.out
@@ -0,0 +1,3 @@
+{"return": {}}
+{"return": {}}
+{"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.19.2

Re: [Qemu-devel] [PULL 0/2] Block layer patches

[Peter Maydell]

On Mon, 3 Dec 2018 at 16:58, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit 83ea23cd207a03c5736be0231acbf7f8b05dbf52:
>
>   i386: hvf: Fix overrun of _decode_tbl1 (2018-12-03 15:09:55 +0000)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to db5e8210adbafe9c6383d8364345a8c545d38e62:
>
>   iotests: simple mirror test with kvm on 1G image (2018-12-03 16:51:53 +0100)
>
> ----------------------------------------------------------------
> Block layer patches:
>
> - mirror: Fix deadlock
>

Applied, thanks.

-- PMM

Re: [Qemu-devel] [qemu-s390x] [PULL 2/2] iotests: simple mirror test with kvm on 1G image

[Christian Borntraeger]

On 04.12.2018 14:49, Christian Borntraeger wrote:
> 
> 
> On 04.12.2018 14:46, Christian Borntraeger wrote:
>> FWIW, this testcase fails with current qemu master on s390:
>>
>> QEMU          -- "/home/cborntra/REPOS/qemu/build/tests/qemu-iotests/../../s390x-softmmu/qemu-system-s390x" -nodefaults -machine accel=qtest
>> QEMU_IMG      -- "/home/cborntra/REPOS/qemu/build/tests/qemu-iotests/../../qemu-img" 
>> QEMU_IO       -- "/home/cborntra/REPOS/qemu/build/tests/qemu-iotests/../../qemu-io"  --cache writeback -f qcow2
>> QEMU_NBD      -- "/home/cborntra/REPOS/qemu/build/tests/qemu-iotests/../../qemu-nbd" 
>> IMGFMT        -- qcow2 (compat=1.1)
>> IMGPROTO      -- file
>> PLATFORM      -- Linux/s390x s38lp08 4.19.0+
>> TEST_DIR      -- /home/cborntra/REPOS/qemu/build/tests/qemu-iotests/scratch
>> SOCKET_SCM_HELPER -- /home/cborntra/REPOS/qemu/build/tests/qemu-iotests/socket_scm_helper
>> 235        
>>  [failed, exit status 1] - output mismatch (see 235.out.bad)
>> --- /home/cborntra/REPOS/qemu/tests/qemu-iotests/235.out	2018-12-04 14:44:27.913714608 +0100
>> +++ /home/cborntra/REPOS/qemu/build/tests/qemu-iotests/235.out.bad	2018-12-04 14:44:51.512958864 +0100
>> @@ -1,3 +1,14 @@
>> -{"return": {}}
>> -{"return": {}}
>> -{"return": {}}
>> +Traceback (most recent call last):
>> +  File "235", line 54, in <module>
>> +    vm.launch()
>> +  File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qemu.py", line 295, in launch
>> +    self._launch()
>> +  File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qemu.py", line 321, in _launch
>> +    self._post_launch()
>> +  File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qemu.py", line 266, in _post_launch
>> +    self._qmp.accept()
>> +  File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line 155, in accept
>> +    self.__sock, _ = self.__sock.accept()
>> +  File "/usr/lib64/python2.7/socket.py", line 206, in accept
>> +    sock, addr = self._sock.accept()
>> +socket.timeout: timed out
>> On 03.12.2018 17:58, Kevin Wolf wrote:
>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>> This test is broken without previous commit fixing dead-lock in mirror.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> Acked-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  tests/qemu-iotests/235     | 76 ++++++++++++++++++++++++++++++++++++++
>>>  tests/qemu-iotests/235.out |  3 ++
>>>  tests/qemu-iotests/group   |  1 +
>>>  3 files changed, 80 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..da044ed34e
>>> --- /dev/null
>>> +++ b/tests/qemu-iotests/235
[...]
>>> +# prepare source image
>>> +qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
>>> +                str(size))
>>> +
>>> +vm = QEMUMachine(iotests.qemu_prog)
>>> +vm.add_args('-machine', 'pc,accel=kvm')

This (pc) clearly does not work on other architectures.
In addition to that, I also need to add -no-shutdown on s390 (see 068 for a similar case)

This hack makes it work for me.

diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
index da044ed34e..05aa641a74 100755
--- a/tests/qemu-iotests/235
+++ b/tests/qemu-iotests/235
@@ -49,7 +49,8 @@ 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('-machine', 'accel=kvm')
+vm.add_args('-no-shutdown')
 vm.add_args('-drive', 'id=src,file=' + disk)
 vm.launch()
 

Re: [Qemu-devel] [qemu-s390x] [PULL 2/2] iotests: simple mirror test with kvm on 1G image

[Kevin Wolf]

Am 05.12.2018 um 09:23 hat Christian Borntraeger geschrieben:
> >>> +# prepare source image
> >>> +qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
> >>> +                str(size))
> >>> +
> >>> +vm = QEMUMachine(iotests.qemu_prog)
> >>> +vm.add_args('-machine', 'pc,accel=kvm')
> 
> This (pc) clearly does not work on other architectures.
> In addition to that, I also need to add -no-shutdown on s390 (see 068 for a similar case)

Leaving out pc definitely makes sense, and the bug still reproduces for
me without it.

I don't understand the -no-shutdown, though. Already for 068, neither
the code nor the commit message when it was added explain why this is
needed.

Can you turn this into a proper patch and add a comment why -no-shutdown
is needed?

Kevin

Re: [Qemu-devel] [qemu-s390x] [PULL 2/2] iotests: simple mirror test with kvm on 1G image

[Christian Borntraeger]

On 05.12.2018 09:46, Kevin Wolf wrote:
> Am 05.12.2018 um 09:23 hat Christian Borntraeger geschrieben:
>>>>> +# prepare source image
>>>>> +qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
>>>>> +                str(size))
>>>>> +
>>>>> +vm = QEMUMachine(iotests.qemu_prog)
>>>>> +vm.add_args('-machine', 'pc,accel=kvm')
>>
>> This (pc) clearly does not work on other architectures.
>> In addition to that, I also need to add -no-shutdown on s390 (see 068 for a similar case)
> 
> Leaving out pc definitely makes sense, and the bug still reproduces for
> me without it.
> 
> I don't understand the -no-shutdown, though. Already for 068, neither
> the code nor the commit message when it was added explain why this is
> needed.
> 
> Can you turn this into a proper patch and add a comment why -no-shutdown
> is needed?

I already sent this patch. The reason is that there is no BIOS in a classical sense
on s390x. If no bootable image (external kernel or from disk) is found, the small boot
bios loads a disabled wait PSW. The default action for that is then shutdown.

Re: [Qemu-devel] [qemu-s390x] [PULL 2/2] iotests: simple mirror test with kvm on 1G image

[Vladimir Sementsov-Ogievskiy]

05.12.2018 12:01, Christian Borntraeger wrote:
> 
> 
> On 05.12.2018 09:46, Kevin Wolf wrote:
>> Am 05.12.2018 um 09:23 hat Christian Borntraeger geschrieben:
>>>>>> +# prepare source image
>>>>>> +qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
>>>>>> +                str(size))
>>>>>> +
>>>>>> +vm = QEMUMachine(iotests.qemu_prog)
>>>>>> +vm.add_args('-machine', 'pc,accel=kvm')
>>>
>>> This (pc) clearly does not work on other architectures.
>>> In addition to that, I also need to add -no-shutdown on s390 (see 068 for a similar case)
>>
>> Leaving out pc definitely makes sense, and the bug still reproduces for
>> me without it.
>>
>> I don't understand the -no-shutdown, though. Already for 068, neither
>> the code nor the commit message when it was added explain why this is
>> needed.
>>
>> Can you turn this into a proper patch and add a comment why -no-shutdown
>> is needed?
> 
> I already sent this patch. The reason is that there is no BIOS in a classical sense
> on s390x. If no bootable image (external kernel or from disk) is found, the small boot
> bios loads a disabled wait PSW. The default action for that is then shutdown.
> 

Is it an option for you just drop the whole line "vm.add_args('-machine', 'pc,accel=kvm')"?
The problem without it for me was that gdb failed to produce full and nice backtrace, but
test worked anyway

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [qemu-s390x] [PULL 2/2] iotests: simple mirror test with kvm on 1G image

[Christian Borntraeger]

On 05.12.2018 13:00, Vladimir Sementsov-Ogievskiy wrote:
> 05.12.2018 12:01, Christian Borntraeger wrote:
>>
>>
>> On 05.12.2018 09:46, Kevin Wolf wrote:
>>> Am 05.12.2018 um 09:23 hat Christian Borntraeger geschrieben:
>>>>>>> +# prepare source image
>>>>>>> +qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
>>>>>>> +                str(size))
>>>>>>> +
>>>>>>> +vm = QEMUMachine(iotests.qemu_prog)
>>>>>>> +vm.add_args('-machine', 'pc,accel=kvm')
>>>>
>>>> This (pc) clearly does not work on other architectures.
>>>> In addition to that, I also need to add -no-shutdown on s390 (see 068 for a similar case)
>>>
>>> Leaving out pc definitely makes sense, and the bug still reproduces for
>>> me without it.
>>>
>>> I don't understand the -no-shutdown, though. Already for 068, neither
>>> the code nor the commit message when it was added explain why this is
>>> needed.
>>>
>>> Can you turn this into a proper patch and add a comment why -no-shutdown
>>> is needed?
>>
>> I already sent this patch. The reason is that there is no BIOS in a classical sense
>> on s390x. If no bootable image (external kernel or from disk) is found, the small boot
>> bios loads a disabled wait PSW. The default action for that is then shutdown.
>>
> 
> Is it an option for you just drop the whole line "vm.add_args('-machine', 'pc,accel=kvm')"?
> The problem without it for me was that gdb failed to produce full and nice backtrace, but
> test worked anyway

In the commid message Vladimir said that kvm is necessary to trigger the problem. 

Re: [Qemu-devel] [qemu-s390x] [PULL 2/2] iotests: simple mirror test with kvm on 1G image

[Vladimir Sementsov-Ogievskiy]

05.12.2018 15:35, Christian Borntraeger wrote:
> 
> 
> On 05.12.2018 13:00, Vladimir Sementsov-Ogievskiy wrote:
>> 05.12.2018 12:01, Christian Borntraeger wrote:
>>>
>>>
>>> On 05.12.2018 09:46, Kevin Wolf wrote:
>>>> Am 05.12.2018 um 09:23 hat Christian Borntraeger geschrieben:
>>>>>>>> +# prepare source image
>>>>>>>> +qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
>>>>>>>> +                str(size))
>>>>>>>> +
>>>>>>>> +vm = QEMUMachine(iotests.qemu_prog)
>>>>>>>> +vm.add_args('-machine', 'pc,accel=kvm')
>>>>>
>>>>> This (pc) clearly does not work on other architectures.
>>>>> In addition to that, I also need to add -no-shutdown on s390 (see 068 for a similar case)
>>>>
>>>> Leaving out pc definitely makes sense, and the bug still reproduces for
>>>> me without it.
>>>>
>>>> I don't understand the -no-shutdown, though. Already for 068, neither
>>>> the code nor the commit message when it was added explain why this is
>>>> needed.
>>>>
>>>> Can you turn this into a proper patch and add a comment why -no-shutdown
>>>> is needed?
>>>
>>> I already sent this patch. The reason is that there is no BIOS in a classical sense
>>> on s390x. If no bootable image (external kernel or from disk) is found, the small boot
>>> bios loads a disabled wait PSW. The default action for that is then shutdown.
>>>
>>
>> Is it an option for you just drop the whole line "vm.add_args('-machine', 'pc,accel=kvm')"?
>> The problem without it for me was that gdb failed to produce full and nice backtrace, but
>> test worked anyway
> 
> In the commid message Vladimir said that kvm is necessary to trigger the problem.
> 

No, I didn't)

and it's in the comment:
# 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)



-- 
Best regards,
Vladimir

Re: [Qemu-devel] [qemu-s390x] [PULL 2/2] iotests: simple mirror test with kvm on 1G image

[Christian Borntraeger]

On 05.12.2018 14:39, Vladimir Sementsov-Ogievskiy wrote:
> 05.12.2018 15:35, Christian Borntraeger wrote:
>>
>>
>> On 05.12.2018 13:00, Vladimir Sementsov-Ogievskiy wrote:
>>> 05.12.2018 12:01, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 05.12.2018 09:46, Kevin Wolf wrote:
>>>>> Am 05.12.2018 um 09:23 hat Christian Borntraeger geschrieben:
>>>>>>>>> +# prepare source image
>>>>>>>>> +qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
>>>>>>>>> +                str(size))
>>>>>>>>> +
>>>>>>>>> +vm = QEMUMachine(iotests.qemu_prog)
>>>>>>>>> +vm.add_args('-machine', 'pc,accel=kvm')
>>>>>>
>>>>>> This (pc) clearly does not work on other architectures.
>>>>>> In addition to that, I also need to add -no-shutdown on s390 (see 068 for a similar case)
>>>>>
>>>>> Leaving out pc definitely makes sense, and the bug still reproduces for
>>>>> me without it.
>>>>>
>>>>> I don't understand the -no-shutdown, though. Already for 068, neither
>>>>> the code nor the commit message when it was added explain why this is
>>>>> needed.
>>>>>
>>>>> Can you turn this into a proper patch and add a comment why -no-shutdown
>>>>> is needed?
>>>>
>>>> I already sent this patch. The reason is that there is no BIOS in a classical sense
>>>> on s390x. If no bootable image (external kernel or from disk) is found, the small boot
>>>> bios loads a disabled wait PSW. The default action for that is then shutdown.
>>>>
>>>
>>> Is it an option for you just drop the whole line "vm.add_args('-machine', 'pc,accel=kvm')"?
>>> The problem without it for me was that gdb failed to produce full and nice backtrace, but
>>> test worked anyway
>>
>> In the commid message Vladimir said that kvm is necessary to trigger the problem.
>>
> 
> No, I didn't)
> 
> and it's in the comment:
> # 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)

Ok, so I would be fine with completely dropping that line.

the patch would then be



"-machine pc" will not work all architectures. Lets fall back to the
default machine by not specifying anything for the machine.

In addition we also need to specify -no-shutdown on s390 as qemu will
exit on guest shutdown. This happens when there is no kernel or bootable
disk on s390.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 tests/qemu-iotests/235 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
index da044ed34e..329da8f0c2 100755
--- a/tests/qemu-iotests/235
+++ b/tests/qemu-iotests/235
@@ -49,7 +49,8 @@ qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
                 str(size))
 
 vm = QEMUMachine(iotests.qemu_prog)
-vm.add_args('-machine', 'pc,accel=kvm')
+if iotests.qemu_default_machine == 's390-ccw-virtio':
+        vm.add_args('-no-shutdown')
 vm.add_args('-drive', 'id=src,file=' + disk)
 vm.launch()
 


Shall I resend a v2?

Re: [Qemu-devel] [qemu-s390x] [PULL 2/2] iotests: simple mirror test with kvm on 1G image

[Vladimir Sementsov-Ogievskiy]

05.12.2018 18:52, Christian Borntraeger wrote:
> 
> 
> On 05.12.2018 14:39, Vladimir Sementsov-Ogievskiy wrote:
>> 05.12.2018 15:35, Christian Borntraeger wrote:
>>>
>>>
>>> On 05.12.2018 13:00, Vladimir Sementsov-Ogievskiy wrote:
>>>> 05.12.2018 12:01, Christian Borntraeger wrote:
>>>>>
>>>>>
>>>>> On 05.12.2018 09:46, Kevin Wolf wrote:
>>>>>> Am 05.12.2018 um 09:23 hat Christian Borntraeger geschrieben:
>>>>>>>>>> +# prepare source image
>>>>>>>>>> +qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
>>>>>>>>>> +                str(size))
>>>>>>>>>> +
>>>>>>>>>> +vm = QEMUMachine(iotests.qemu_prog)
>>>>>>>>>> +vm.add_args('-machine', 'pc,accel=kvm')
>>>>>>>
>>>>>>> This (pc) clearly does not work on other architectures.
>>>>>>> In addition to that, I also need to add -no-shutdown on s390 (see 068 for a similar case)
>>>>>>
>>>>>> Leaving out pc definitely makes sense, and the bug still reproduces for
>>>>>> me without it.
>>>>>>
>>>>>> I don't understand the -no-shutdown, though. Already for 068, neither
>>>>>> the code nor the commit message when it was added explain why this is
>>>>>> needed.
>>>>>>
>>>>>> Can you turn this into a proper patch and add a comment why -no-shutdown
>>>>>> is needed?
>>>>>
>>>>> I already sent this patch. The reason is that there is no BIOS in a classical sense
>>>>> on s390x. If no bootable image (external kernel or from disk) is found, the small boot
>>>>> bios loads a disabled wait PSW. The default action for that is then shutdown.
>>>>>
>>>>
>>>> Is it an option for you just drop the whole line "vm.add_args('-machine', 'pc,accel=kvm')"?
>>>> The problem without it for me was that gdb failed to produce full and nice backtrace, but
>>>> test worked anyway
>>>
>>> In the commid message Vladimir said that kvm is necessary to trigger the problem.
>>>
>>
>> No, I didn't)
>>
>> and it's in the comment:
>> # 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)
> 
> Ok, so I would be fine with completely dropping that line.
> 
> the patch would then be
> 
> 
> 
> "-machine pc" will not work all architectures. Lets fall back to the
> default machine by not specifying anything for the machine.
> 
> In addition we also need to specify -no-shutdown on s390 as qemu will
> exit on guest shutdown. This happens when there is no kernel or bootable
> disk on s390.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>   tests/qemu-iotests/235 | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
> index da044ed34e..329da8f0c2 100755
> --- a/tests/qemu-iotests/235
> +++ b/tests/qemu-iotests/235
> @@ -49,7 +49,8 @@ qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
>                   str(size))
>   
>   vm = QEMUMachine(iotests.qemu_prog)
> -vm.add_args('-machine', 'pc,accel=kvm')
> +if iotests.qemu_default_machine == 's390-ccw-virtio':
> +        vm.add_args('-no-shutdown')
>   vm.add_args('-drive', 'id=src,file=' + disk)
>   vm.launch()
>   
> 
> 
> Shall I resend a v2?
> 

so, we need -no-shutdown even if we drop kvm? I hoped that not.. Hmm. grep points only to one iotest doing the same about no-shutdown - 068..


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [qemu-s390x] [PULL 2/2] iotests: simple mirror test with kvm on 1G image

[Christian Borntraeger]

On 05.12.2018 17:09, Vladimir Sementsov-Ogievskiy wrote:
> 05.12.2018 18:52, Christian Borntraeger wrote:
>>
>>
>> On 05.12.2018 14:39, Vladimir Sementsov-Ogievskiy wrote:
>>> 05.12.2018 15:35, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 05.12.2018 13:00, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 05.12.2018 12:01, Christian Borntraeger wrote:
>>>>>>
>>>>>>
>>>>>> On 05.12.2018 09:46, Kevin Wolf wrote:
>>>>>>> Am 05.12.2018 um 09:23 hat Christian Borntraeger geschrieben:
>>>>>>>>>>> +# prepare source image
>>>>>>>>>>> +qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
>>>>>>>>>>> +                str(size))
>>>>>>>>>>> +
>>>>>>>>>>> +vm = QEMUMachine(iotests.qemu_prog)
>>>>>>>>>>> +vm.add_args('-machine', 'pc,accel=kvm')
>>>>>>>>
>>>>>>>> This (pc) clearly does not work on other architectures.
>>>>>>>> In addition to that, I also need to add -no-shutdown on s390 (see 068 for a similar case)
>>>>>>>
>>>>>>> Leaving out pc definitely makes sense, and the bug still reproduces for
>>>>>>> me without it.
>>>>>>>
>>>>>>> I don't understand the -no-shutdown, though. Already for 068, neither
>>>>>>> the code nor the commit message when it was added explain why this is
>>>>>>> needed.
>>>>>>>
>>>>>>> Can you turn this into a proper patch and add a comment why -no-shutdown
>>>>>>> is needed?
>>>>>>
>>>>>> I already sent this patch. The reason is that there is no BIOS in a classical sense
>>>>>> on s390x. If no bootable image (external kernel or from disk) is found, the small boot
>>>>>> bios loads a disabled wait PSW. The default action for that is then shutdown.
>>>>>>
>>>>>
>>>>> Is it an option for you just drop the whole line "vm.add_args('-machine', 'pc,accel=kvm')"?
>>>>> The problem without it for me was that gdb failed to produce full and nice backtrace, but
>>>>> test worked anyway
>>>>
>>>> In the commid message Vladimir said that kvm is necessary to trigger the problem.
>>>>
>>>
>>> No, I didn't)
>>>
>>> and it's in the comment:
>>> # 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)
>>
>> Ok, so I would be fine with completely dropping that line.
>>
>> the patch would then be
>>
>>
>>
>> "-machine pc" will not work all architectures. Lets fall back to the
>> default machine by not specifying anything for the machine.
>>
>> In addition we also need to specify -no-shutdown on s390 as qemu will
>> exit on guest shutdown. This happens when there is no kernel or bootable
>> disk on s390.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>   tests/qemu-iotests/235 | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
>> index da044ed34e..329da8f0c2 100755
>> --- a/tests/qemu-iotests/235
>> +++ b/tests/qemu-iotests/235
>> @@ -49,7 +49,8 @@ qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
>>                   str(size))
>>   
>>   vm = QEMUMachine(iotests.qemu_prog)
>> -vm.add_args('-machine', 'pc,accel=kvm')
>> +if iotests.qemu_default_machine == 's390-ccw-virtio':
>> +        vm.add_args('-no-shutdown')
>>   vm.add_args('-drive', 'id=src,file=' + disk)
>>   vm.launch()
>>   
>>
>>
>> Shall I resend a v2?
>>
> 
> so, we need -no-shutdown even if we drop kvm? I hoped that not.. Hmm. grep points only to one iotest doing the same about no-shutdown - 068..

Yes, without that we fail with

--- /home/cborntra/REPOS/qemu/tests/qemu-iotests/235.out	2018-12-04 14:44:27.913714608 +0100
+++ /home/cborntra/REPOS/qemu/build/tests/qemu-iotests/235.out.bad	2018-12-05 17:23:05.601827490 +0100
@@ -1,3 +1,13 @@
 {"return": {}}
 {"return": {}}
 {"return": {}}
+Traceback (most recent call last):
+  File "235", line 70, in <module>
+    vm.event_wait('BLOCK_JOB_READY', timeout=10.0)
+  File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qemu.py", line 436, in event_wait
+    event = self._qmp.pull_event(wait=timeout)
+  File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line 216, in pull_event
+    self.__get_events(wait)
+  File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line 128, in __get_events
+    raise QMPConnectError("Error while reading from socket")
+qmp.qmp.QMPConnectError: Error while reading from socket
Failures: 235
Failed 1 of 1 tests

Re: [Qemu-devel] [qemu-s390x] [PULL 2/2] iotests: simple mirror test with kvm on 1G image

[Christian Borntraeger]

On 05.12.2018 17:09, Vladimir Sementsov-Ogievskiy wrote:
> 05.12.2018 18:52, Christian Borntraeger wrote:
>>
>>
>> On 05.12.2018 14:39, Vladimir Sementsov-Ogievskiy wrote:
>>> 05.12.2018 15:35, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 05.12.2018 13:00, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 05.12.2018 12:01, Christian Borntraeger wrote:
>>>>>>
>>>>>>
>>>>>> On 05.12.2018 09:46, Kevin Wolf wrote:
>>>>>>> Am 05.12.2018 um 09:23 hat Christian Borntraeger geschrieben:
>>>>>>>>>>> +# prepare source image
>>>>>>>>>>> +qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
>>>>>>>>>>> +                str(size))
>>>>>>>>>>> +
>>>>>>>>>>> +vm = QEMUMachine(iotests.qemu_prog)
>>>>>>>>>>> +vm.add_args('-machine', 'pc,accel=kvm')
>>>>>>>>
>>>>>>>> This (pc) clearly does not work on other architectures.
>>>>>>>> In addition to that, I also need to add -no-shutdown on s390 (see 068 for a similar case)
>>>>>>>
>>>>>>> Leaving out pc definitely makes sense, and the bug still reproduces for
>>>>>>> me without it.
>>>>>>>
>>>>>>> I don't understand the -no-shutdown, though. Already for 068, neither
>>>>>>> the code nor the commit message when it was added explain why this is
>>>>>>> needed.
>>>>>>>
>>>>>>> Can you turn this into a proper patch and add a comment why -no-shutdown
>>>>>>> is needed?
>>>>>>
>>>>>> I already sent this patch. The reason is that there is no BIOS in a classical sense
>>>>>> on s390x. If no bootable image (external kernel or from disk) is found, the small boot
>>>>>> bios loads a disabled wait PSW. The default action for that is then shutdown.
>>>>>>
>>>>>
>>>>> Is it an option for you just drop the whole line "vm.add_args('-machine', 'pc,accel=kvm')"?
>>>>> The problem without it for me was that gdb failed to produce full and nice backtrace, but
>>>>> test worked anyway
>>>>
>>>> In the commid message Vladimir said that kvm is necessary to trigger the problem.
>>>>
>>>
>>> No, I didn't)
>>>
>>> and it's in the comment:
>>> # 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)
>>
>> Ok, so I would be fine with completely dropping that line.
>>
>> the patch would then be
>>
>>
>>
>> "-machine pc" will not work all architectures. Lets fall back to the
>> default machine by not specifying anything for the machine.
>>
>> In addition we also need to specify -no-shutdown on s390 as qemu will
>> exit on guest shutdown. This happens when there is no kernel or bootable
>> disk on s390.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>   tests/qemu-iotests/235 | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
>> index da044ed34e..329da8f0c2 100755
>> --- a/tests/qemu-iotests/235
>> +++ b/tests/qemu-iotests/235
>> @@ -49,7 +49,8 @@ qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
>>                   str(size))
>>   
>>   vm = QEMUMachine(iotests.qemu_prog)
>> -vm.add_args('-machine', 'pc,accel=kvm')
>> +if iotests.qemu_default_machine == 's390-ccw-virtio':
>> +        vm.add_args('-no-shutdown')
>>   vm.add_args('-drive', 'id=src,file=' + disk)
>>   vm.launch()
>>   
>>
>>
>> Shall I resend a v2?
>>
> 
> so, we need -no-shutdown even if we drop kvm? I hoped that not.. Hmm. grep points only to one iotest doing the same about no-shutdown - 068..

Kevin, shall I send the above patch as v2?

Re: [Qemu-devel] [qemu-s390x] [PULL 2/2] iotests: simple mirror test with kvm on 1G image

[Kevin Wolf]

Am 06.12.2018 um 12:05 hat Christian Borntraeger geschrieben:
> 
> 
> On 05.12.2018 17:09, Vladimir Sementsov-Ogievskiy wrote:
> > 05.12.2018 18:52, Christian Borntraeger wrote:
> >>
> >>
> >> On 05.12.2018 14:39, Vladimir Sementsov-Ogievskiy wrote:
> >>> 05.12.2018 15:35, Christian Borntraeger wrote:
> >>>>
> >>>>
> >>>> On 05.12.2018 13:00, Vladimir Sementsov-Ogievskiy wrote:
> >>>>> 05.12.2018 12:01, Christian Borntraeger wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 05.12.2018 09:46, Kevin Wolf wrote:
> >>>>>>> Am 05.12.2018 um 09:23 hat Christian Borntraeger geschrieben:
> >>>>>>>>>>> +# prepare source image
> >>>>>>>>>>> +qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
> >>>>>>>>>>> +                str(size))
> >>>>>>>>>>> +
> >>>>>>>>>>> +vm = QEMUMachine(iotests.qemu_prog)
> >>>>>>>>>>> +vm.add_args('-machine', 'pc,accel=kvm')
> >>>>>>>>
> >>>>>>>> This (pc) clearly does not work on other architectures.
> >>>>>>>> In addition to that, I also need to add -no-shutdown on s390 (see 068 for a similar case)
> >>>>>>>
> >>>>>>> Leaving out pc definitely makes sense, and the bug still reproduces for
> >>>>>>> me without it.
> >>>>>>>
> >>>>>>> I don't understand the -no-shutdown, though. Already for 068, neither
> >>>>>>> the code nor the commit message when it was added explain why this is
> >>>>>>> needed.
> >>>>>>>
> >>>>>>> Can you turn this into a proper patch and add a comment why -no-shutdown
> >>>>>>> is needed?
> >>>>>>
> >>>>>> I already sent this patch. The reason is that there is no BIOS in a classical sense
> >>>>>> on s390x. If no bootable image (external kernel or from disk) is found, the small boot
> >>>>>> bios loads a disabled wait PSW. The default action for that is then shutdown.
> >>>>>>
> >>>>>
> >>>>> Is it an option for you just drop the whole line "vm.add_args('-machine', 'pc,accel=kvm')"?
> >>>>> The problem without it for me was that gdb failed to produce full and nice backtrace, but
> >>>>> test worked anyway
> >>>>
> >>>> In the commid message Vladimir said that kvm is necessary to trigger the problem.
> >>>>
> >>>
> >>> No, I didn't)
> >>>
> >>> and it's in the comment:
> >>> # 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)
> >>
> >> Ok, so I would be fine with completely dropping that line.
> >>
> >> the patch would then be
> >>
> >>
> >>
> >> "-machine pc" will not work all architectures. Lets fall back to the
> >> default machine by not specifying anything for the machine.
> >>
> >> In addition we also need to specify -no-shutdown on s390 as qemu will
> >> exit on guest shutdown. This happens when there is no kernel or bootable
> >> disk on s390.
> >>
> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> ---
> >>   tests/qemu-iotests/235 | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
> >> index da044ed34e..329da8f0c2 100755
> >> --- a/tests/qemu-iotests/235
> >> +++ b/tests/qemu-iotests/235
> >> @@ -49,7 +49,8 @@ qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
> >>                   str(size))
> >>   
> >>   vm = QEMUMachine(iotests.qemu_prog)
> >> -vm.add_args('-machine', 'pc,accel=kvm')
> >> +if iotests.qemu_default_machine == 's390-ccw-virtio':
> >> +        vm.add_args('-no-shutdown')
> >>   vm.add_args('-drive', 'id=src,file=' + disk)
> >>   vm.launch()
> >>   
> >>
> >>
> >> Shall I resend a v2?
> >>
> > 
> > so, we need -no-shutdown even if we drop kvm? I hoped that not..
> > Hmm. grep points only to one iotest doing the same about no-shutdown
> > - 068..
> 
> Kevin, shall I send the above patch as v2?

Don't bother, I think v1 is fine. By specifying kvm explicitly, it's at
least clear that we're not using qtest like in other tests.

Kevin