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

[Kevin Wolf]

Am 01.07.2015 um 16:45 hat Stefan Hajnoczi geschrieben:
> 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>

This looks like a backend-specific problem with raw-posix. Wouldn't it
make more sense to either let raw-posix split requests it can't handle
or introduce a bs->iov_max instead of spreading knowledge about specific
backends into all callers of the block layer?

I'm not objecting to taking this patch for now to fix the bug in 2.4,
but I don't think it's the proper solution.

Kevin

>  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) {
> +            trace_mirror_break_iov_max(s, nb_chunks, added_chunks);
> +            break;
> +        }
>  
>          /* We have enough free space to copy these sectors.  */
>          bitmap_set(s->in_flight_bitmap, next_chunk, added_chunks);
> diff --git a/trace-events b/trace-events
> index 52b7efa..943cd0c 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -94,6 +94,7 @@ mirror_yield(void *s, int64_t cnt, int buf_free_count, int 
> in_flight) "s %p dirt
>  mirror_yield_in_flight(void *s, int64_t sector_num, int in_flight) "s %p 
> sector_num %"PRId64" in_flight %d"
>  mirror_yield_buf_busy(void *s, int nb_chunks, int in_flight) "s %p requested 
> chunks %d in_flight %d"
>  mirror_break_buf_busy(void *s, int nb_chunks, int in_flight) "s %p requested 
> chunks %d in_flight %d"
> +mirror_break_iov_max(void *s, int nb_chunks, int added_chunks) "s %p 
> requested chunks %d added_chunks %d"
>  
>  # block/backup.c
>  backup_do_cow_enter(void *job, int64_t start, int64_t sector_num, int 
> nb_sectors) "job %p start %"PRId64" sector_num %"PRId64" nb_sectors %d"
> -- 
> 2.4.3
> 
>