Re: [Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Peter Lieven <pl@kamp.de>]

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