From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46167) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VXEc3-0005DI-2B for qemu-devel@nongnu.org; Fri, 18 Oct 2013 14:17:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VXEbu-0008WI-53 for qemu-devel@nongnu.org; Fri, 18 Oct 2013 14:17:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:24715) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VXEbt-0008Vg-TM for qemu-devel@nongnu.org; Fri, 18 Oct 2013 14:17:18 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r9IIHH99022712 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 18 Oct 2013 14:17:17 -0400 Message-ID: <52617B29.9040104@redhat.com> Date: Fri, 18 Oct 2013 20:17:13 +0200 From: Max Reitz MIME-Version: 1.0 References: <1381804911-3664-1-git-send-email-famz@redhat.com> <525EDEB9.5030603@redhat.com> <20131017062852.GA6258@T430s.nay.redhat.com> In-Reply-To: <20131017062852.GA6258@T430s.nay.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] qemu-iotests: fix 030 for faster machines List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: famz@redhat.com Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On 2013-10-17 08:28, Fam Zheng wrote: > On Wed, 10/16 20:45, Max Reitz wrote: >> On 2013-10-15 04:41, Fam Zheng wrote: >>> If the block job completes too fast, the test can fail. Change the >>> numbers so the qmp events are more stably captured by the script. >>> >>> A sleep is removed for the same reason. >>> >>> Signed-off-by: Fam Zheng >>> --- >>> tests/qemu-iotests/030 | 11 +++++------ >>> 1 file changed, 5 insertions(+), 6 deletions(-) >>> >>> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030 >>> index ae56f3b..188b182 100755 >>> --- a/tests/qemu-iotests/030 >>> +++ b/tests/qemu-iotests/030 >>> @@ -403,14 +403,13 @@ class TestStreamStop(iotests.QMPTestCase): >>> result =3D self.vm.qmp('block-stream', device=3D'drive0') >>> self.assert_qmp(result, 'return', {}) >>> - time.sleep(0.1) >> Hm, I'm not sure whether removing the sleep actually removes the >> underlying race condition=E2=80=A6 It should work in most cases and th= e >> foreseeable future, though. >> >>> events =3D self.vm.get_qmp_events(wait=3DFalse) >>> self.assertEqual(events, [], 'unexpected QMP event: %s' % e= vents) >>> self.cancel_and_wait() >>> class TestSetSpeed(iotests.QMPTestCase): >>> - image_len =3D 80 * 1024 * 1024 # MB >>> + image_len =3D 512 * 1024 * 1024 # MB >>> def setUp(self): >>> qemu_img('create', backing_img, str(TestSetSpeed.image_len)= ) >>> @@ -457,23 +456,23 @@ class TestSetSpeed(iotests.QMPTestCase): >>> self.assert_qmp(result, 'return[0]/device', 'drive0') >>> self.assert_qmp(result, 'return[0]/speed', 0) >>> - result =3D self.vm.qmp('block-job-set-speed', device=3D'driv= e0', speed=3D8 * 1024 * 1024) >>> + result =3D self.vm.qmp('block-job-set-speed', device=3D'driv= e0', speed=3D8 * 1024) >> So the limit was already 8 MB/s? Doesn't this mean that the job >> should have taken 10 seconds anyway? Sounds to me like the block job >> speed is basically disregarded anyway. > No, see below... > >> If I re-add the sleep you removed in this patch, this test fails >> again for me. This further suggests block-job-set-speed to be kind >> of a no-op and the changes concerning the image length and the block >> job speed not really contributing to fixing the test. >> >> So I think removing the sleep is all that would have to be done >> right now. OTOH, this is not really a permanent fix, either (the >> fundamental race condition remains). Furthermore, I guess there is >> some reason for having a sleep there - else it would not exist in >> the first place (and it apparently already caused problems some time >> ago which were "fixed" by replacing the previous "sleep(1)" by >> "sleep(0.1)"). >> >> All in all, if someone can assure me of the uneccessity of the sleep >> in question, I think removing it is all that's needed. >> >> Max >> > Both failure cases are just that setting speed or checking status comes= too > late: the streaming finishes or goes close to finish in negligible no t= ime once > the job is started. In other words dropping the speed change but only i= ncrease > image_len and remove sleep will fix it for me too. Ah, sorry, I missed that those failures are two seperate test cases and=20 both changes are basically independent of each other. Sorry, my fault. Hm, well, but I'm still not happy with removing the sleep. I've thought=20 of a different solution myself and didn't find any either=E2=80=A6 But th= e fact=20 remains that there are three things that can happen: First, the block job might finish before the cancelling QMP command gets=20 sent anyway. The test script and qemu are independent of each other, so=20 this may still happen (although the block device has to be really fast=20 for that to happen, I guess). Second, I'd still like an explanation why the sleep is indeed=20 unnecessary. I guess its purpose is to have the block job actually=20 running before cancelling it =E2=80=93 removing the sleep might defeat th= at=20 purpose, though I don't know how bad this is. Third, since qemu is indeed running independently of the test script,=20 the blockjob is in fact running and has not yet finished by the time it=20 gets cancelled. This would be the desired result. I admit that the first outcome is impossible for all realistic=20 scenarios. However, the second one is what's making me feel uncomfortable. Max