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