From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55563) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b7K9d-0000Tq-Vx for qemu-devel@nongnu.org; Mon, 30 May 2016 06:10:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b7K9T-00018y-Pq for qemu-devel@nongnu.org; Mon, 30 May 2016 06:10:36 -0400 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:57767 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b7K9T-00018j-G6 for qemu-devel@nongnu.org; Mon, 30 May 2016 06:10:27 -0400 References: <413c4af8-ba30-bfbf-9a84-d1d92ddac0e4@redhat.com> <20160526083016.GD31052@ad.usersys.redhat.com> <4b26422f-9320-31a6-9dca-534bf0ea6086@redhat.com> <20160527003602.GG31052@ad.usersys.redhat.com> <20160527085559.GC5467@noname.redhat.com> <574BDCC4.8000704@kamp.de> <20160530082412.GB4436@noname.str.redhat.com> <574C0822.6040004@kamp.de> <20160530094719.GC4436@noname.str.redhat.com> <574C0D92.5090302@kamp.de> <20160530100610.GD4436@noname.str.redhat.com> From: Peter Lieven Message-ID: <574C118E.5000100@kamp.de> Date: Mon, 30 May 2016 12:10:22 +0200 MIME-Version: 1.0 In-Reply-To: <20160530100610.GD4436@noname.str.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Fam Zheng , Paolo Bonzini , qemu-block@nongnu.org, qemu-devel@nongnu.org, stefanha@redhat.com, mreitz@redhat.com Am 30.05.2016 um 12:06 schrieb Kevin Wolf: > Am 30.05.2016 um 11:53 hat Peter Lieven geschrieben: >> Am 30.05.2016 um 11:47 schrieb Kevin Wolf: >>> Am 30.05.2016 um 11:30 hat Peter Lieven geschrieben: >>>> Am 30.05.2016 um 10:24 schrieb Kevin Wolf: >>>>> Am 30.05.2016 um 08:25 hat Peter Lieven geschrieben: >>>>>> Am 27.05.2016 um 10:55 schrieb Kevin Wolf: >>>>>>> Am 27.05.2016 um 02:36 hat Fam Zheng geschrieben: >>>>>>>> On Thu, 05/26 11:20, Paolo Bonzini wrote: >>>>>>>>> On 26/05/2016 10:30, Fam Zheng wrote: >>>>>>>>>>>> This doesn't look too wrong... Should the right sequence of events be >>>>>>>>>>>> head/after_head or head/after_tail? It's probably simplest to just emit >>>>>>>>>>>> all four events. >>>>>>>>>> I've no idea. (That's why I leaned towards fixing the test case). >>>>>>>>> Well, fixing the testcase means knowing what events should be emitted. >>>>>>>>> >>>>>>>>> QEMU with Peter's patch emits head/after_head. If the right one is >>>>>>>>> head/after_tail, _both QEMU and the testcase_ need to be adjusted. Your >>>>>>>>> patch keeps the backwards-compatible route. >>>>>>>> Yes, I mean I was not very convinced in tweaking the events at all: each pair >>>>>>>> of them has been emitted around bdrv_aligned_preadv(), and the new branch >>>>>>>> doesn't do it anymore. So I don't see a reason to add events here. >>>>>>> Yes, if you can assume that anyone who uses the debug events know >>>>>>> exactly what the code looks like, adding the events here is pointless >>>>>>> because TAIL, AFTER_TAIL and for the greatest part also AFTER_HEAD are >>>>>>> essentially the same then. >>>>>>> >>>>>>> Having TAIL before the qiov change and AFTER_TAIL afterwards doesn't >>>>>>> make any difference, they could (and should) be called immediately one >>>>>>> after another if we wanted to keep the behaviour. >>>>>>> >>>>>>> I would agree that we should take a look at the test case and what it >>>>>>> actually wants to achieve before we can decide whether AFTER_HEAD and >>>>>>> TAIL/AFTER_TAIL would be the same (the former could trigger earlier if >>>>>>> there are two requests and only one is unaligned at the tail). Maybe we >>>>>>> even need to extend the test case now so that both paths (explicit read >>>>>>> of the tail and the shortcut) are covered. >>>>>> The part that actually blocks in 077 is >>>>>> >>>>>> # Sequential RMW requests on the same physical sector >>>>>> >>>>>> its expecting all 4 events around the RMW cycle. >>>>>> >>>>>> However, it seems that also other parts of 077 would need an adjustment >>>>>> and the output might differ depending on the alignment. So I guess we >>>>>> have to emit the events if we don't want to recode the whole 077 and make >>>>>> it aware of the alignment. >>>>> Yes, but my point is that we may need to rework 077 anyway if we don't >>>>> only want to make it pass again, but to cover all relevant paths, too. >>>>> We got a new code path and it's unlikely that the existing tests covered >>>>> both the old code path and the new one. >>>> So you would postpone this patch until 077 is reworked? >>>> I found this one a nice improvement and 077 might take some time. >>> The problem with "we'll rework the tests later" is always that it >>> doesn't happen if the patches for the functional parts and a workaround >>> for the test case are merged. >>> >>> I don't think that making 077 cover both cases should be hard or take >>> much time, it just needs to be done. If all the time for writing emails >>> in this thread had been used to work on the test case, it would already >>> be done. >> Understood. If you can give a hint how to get the value of the align >> parameter into the test script I can try. Otherwise the test will fail >> also if any block driver has an align value that is not equal to 512. > The test case already uses blkdebug to enforce a specific align value > (which is 4096 in this test case, not 512): > > echo "open -o driver=$IMGFMT,file.align=4k blkdebug::$TEST_IMG" Sorry, I missed that. Then I will try to fix 077. Peter