On 04.07.2016 16:30, Sascha Silbe wrote:
> Dear Max,
>
> Max Reitz <mre...@redhat.com> writes:
>
>> On 28.06.2016 17:28, Sascha Silbe wrote:
> [block/mirror.c]
>>> @@ -416,7 +416,9 @@ static uint64_t coroutine_fn
>>> mirror_iteration(MirrorBlockJob *s)
>>> assert(io_sectors);
>>> sector_num += io_sectors;
>>> nb_chunks -= DIV_ROUND_UP(io_sectors, sectors_per_chunk);
>>> - delay_ns += ratelimit_calculate_delay(&s->limit, io_sectors);
>>> + if (s->common.speed) {
>>> + delay_ns = ratelimit_calculate_delay(&s->limit, io_sectors);
>>> + }
>>
>> Hmm... Was it a bug that ratelimit_calculate_delay() was called
>> unconditionally before?
>
> One could argue either way. It happened to work because
> ratelimit_calculate_delay() only delayed the _second_ time (within one
> time slice) the quota was exceeded. With zero duration time slices,
> there never was a second time.
>
> With the new implementation we would divide by zero when slice_quota is
> 0, so we need to guard against that. Most callers already did, only
> mirror_iteration() needed to be adjusted.
>
>
> [...]
> [include/qemu/ratelimit.h]
>>> static inline int64_t ratelimit_calculate_delay(RateLimit *limit, uint64_t
>>> n)
>>> {
>>> int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>>> + uint64_t delay_slices;
>>>
>>> - if (limit->next_slice_time < now) {
>>> - limit->next_slice_time = now + limit->slice_ns;
>>> + assert(limit->slice_quota && limit->slice_ns);
>>> +
>>> + if (limit->slice_end_time < now) {
>>> + /* Previous, possibly extended, time slice finished; reset the
>>> + * accounting. */
>>> + limit->slice_start_time = now;
>>> + limit->slice_end_time = now + limit->slice_ns;
>>> limit->dispatched = 0;
>>> }
>>> - if (limit->dispatched == 0 || limit->dispatched + n <=
>>> limit->slice_quota) {
>>> - limit->dispatched += n;
>>> +
>>> + limit->dispatched += n;
>>> + if (limit->dispatched < limit->slice_quota) {
>>
>> Nitpick: This should probably stay <=.
>
> This is a subtle edge case. Previously, when limit->dispatched ==
> limit->slice_quota, we returned 0 so that the _current_ request (which
> is still within quota) wouldn't be delayed. Now, we return a delay so
> that the _next_ request (which would be over quota) will be delayed.Hm, but that depends on the size of the next request. Of course, if we get limit->dispatched == limit->slice_quota we know for sure that we need to delay the next request. But if we get limit->dispatched == limit->slice_quota - 1... Then we probably also have to delay it, but we don't know for sure. So I think it would be better to have small but consistent systematic error here, i.e. that we will not delay the last request even though we should. Or you could insert a delay after the last request in all block jobs, too. Or did I fail to understand the issue? I'm not sure. > [...] >>> + /* Quota exceeded. Calculate the next time slice we may start >>> + * sending data again. */ >>> + delay_slices = (limit->dispatched + limit->slice_quota - 1) / >>> + limit->slice_quota; >>> + limit->slice_end_time = limit->slice_start_time + >>> + delay_slices * limit->slice_ns; >> >> I think it would make sense to make this a floating point calculation. > > Then we'd have fully variable length time slices, instead of just > "occupying" multiple fixed-length time slices with a single > request. Maybe that would be even better, or maybe we'd cause other > interesting things to happen (think interactions with the scheduler). :-) > As > this code will hopefully disappear during the 2.8 time line anyway, I'd > prefer to go with the lowest risk option that is enough to fix the race > conditions encountered by the test suite. OK with me. Max >> If you don't agree, though: >> >> Reviewed-by: Max Reitz <mre...@redhat.com> > > Thanks for the review! > > Sascha >
signature.asc
Description: OpenPGP digital signature
