Re: [Qemu-devel] [Qemu-block] [PATCH] block/mirror: limit qiov to IOV_MAX elements

[Stefan Hajnoczi]

On Wed, Jul 1, 2015 at 3:47 PM, Paolo Bonzini <address@hidden> wrote:
> On 01/07/2015 16:45, Stefan Hajnoczi wrote:
>> If mirror has more free buffers than IOV_MAX, preadv(2)/pwritev(2)
>> EINVAL failures may be encountered.
>>
>> It is possible to trigger this by setting granularity to a low value
>> like 8192.
>>
>> This patch stops appending chunks once IOV_MAX is reached.
>>
>> The spurious EINVAL failure can be reproduced with a qcow2 image file
>> and the following QMP invocation:
>>
>>   qmp.command('drive-mirror', device='virtio0', target='/tmp/r7.s1',
>>               granularity=8192, sync='full', mode='absolute-paths',
>>               format='raw')
>>
>> While the guest is running dd if=/dev/zero of=/var/tmp/foo oflag=direct
>> bs=4k.
>>
>> Cc: Jeff Cody <address@hidden>
>> Signed-off-by: Stefan Hajnoczi <address@hidden>
>> ---
>>  block/mirror.c | 4 ++++
>>  trace-events   | 1 +
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 048e452..985ad00 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -241,6 +241,10 @@ static uint64_t coroutine_fn 
>> mirror_iteration(MirrorBlockJob *s)
>>              trace_mirror_break_buf_busy(s, nb_chunks, s->in_flight);
>>              break;
>>          }
>> +        if (IOV_MAX < nb_chunks + added_chunks) {
>
> No Yoda conditions... apart from that,
>
> Reviewed-by: Paolo Bonzini <address@hidden>

I found it annoying to write it backwards too, but it's for consistency:

  if (s->buf_free_count < nb_chunks + added_chunks) {
      trace_mirror_break_buf_busy(s, nb_chunks, s->in_flight);
      break;
  }
  if (IOV_MAX < nb_chunks + added_chunks) {
      trace_mirror_break_iov_max(s, nb_chunks, added_chunks);
      break;
  }

It's the same type of check as s->buf_free_count (which isn't modified
by this loop either so it's a yoda conditional).