Re: [Qemu-devel] [PATCH 1/1] mirror: fix request throttling in drive-mirror

["Denis V. Lunev" <den@openvz.org>]

On 06/29/2016 07:08 PM, Max Reitz wrote:
> On 22.06.2016 14:35, Denis V. Lunev wrote:
>> There are 2 deficiencies here:
>> - mirror_iteration could start several requests inside. Thus we could
>>    simply have more in_flight requests than MAX_IN_FLIGHT.
>> - keeping this in mind throttling in mirror_run which is checking
>>    s->in_flight == MAX_IN_FLIGHT is wrong.
>>
>> The patch adds the check and throttling into mirror_iteration and fixes
>> the check in mirror_run() to be sure.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Jeff Cody <jcody@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/mirror.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index a04ed9c..e881ef6 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -399,6 +399,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>>               }
>>           }
>>   
>> +        while (s->in_flight >= MAX_IN_FLIGHT) {
>> +            trace_mirror_yield_in_flight(s, sector_num, s->in_flight);
>> +            mirror_wait_for_io(s);
>> +        }
>> +
>>           mirror_clip_sectors(s, sector_num, &io_sectors);
>>           switch (mirror_method) {
>>           case MIRROR_METHOD_COPY:
>> @@ -634,7 +639,7 @@ static void coroutine_fn mirror_run(void *opaque)
>>            */
>>           if (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - last_pause_ns < SLICE_TIME &&
>>               s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
>> -            if (s->in_flight == MAX_IN_FLIGHT || s->buf_free_count == 0 ||
>> +            if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
>>                   (cnt == 0 && s->in_flight > 0)) {
>>                   trace_mirror_yield(s, s->in_flight, s->buf_free_count, cnt);
>>                   mirror_wait_for_io(s);
>>
> Using >= seems fine to me, but with the first hunk applied I can't
> imagine how s->in_flight should grow beyond MAX_IN_FLIGHT. Don't get me
> wrong, I myself like to use >= even where the > case should never
> happen, I'm just wondering if I'm missing something here.
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
I think that this should not happen anymore,
but I'd like to stay on the safe side.

Den