[Qemu-devel] [PATCH] block: prevent multiwrite_merge from creating too large iovecs

[Qemu-devel] [PATCH] block: prevent multiwrite_merge from creating too large iovecs

[Christoph Hellwig]

If we go over the maximum number of iovecs support by syscall we get
back EINVAL from the kernel which translate to I/O errors for the guest.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: qemu/block.c
===================================================================
--- qemu.orig/block.c	2010-01-19 22:10:19.797003226 +0100
+++ qemu/block.c	2010-01-19 22:11:08.226005767 +0100
@@ -1711,6 +1711,10 @@ static int multiwrite_merge(BlockDriverS
             merge = bs->drv->bdrv_merge_requests(bs, &reqs[outidx], &reqs[i]);
         }
 
+        if (reqs[outidx].qiov->niov + reqs[i].qiov->niov + 1 > IOV_MAX) {
+            merge = 0;
+        }
+
         if (merge) {
             size_t size;
             QEMUIOVector *qiov = qemu_mallocz(sizeof(*qiov));

Re: [Qemu-devel] [PATCH] block: prevent multiwrite_merge from creating too large iovecs

[Kevin Wolf]

Am 19.01.2010 22:15, schrieb Christoph Hellwig:
> If we go over the maximum number of iovecs support by syscall we get
> back EINVAL from the kernel which translate to I/O errors for the guest.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Is this really enough? We don't check for IOV_MAX in any other place, so
can't we get a too big request directly from virtio-blk?

What about handling it transparently in qemu_pwritev/preadv and
laio_submit? Logically, it's a limitation of the backend anyway and not
a generic restriction.

To underline that it's a backend/platform dependent thing: Your patch
breaks the mingw build for me.

Kevin

Re: [Qemu-devel] [PATCH] block: prevent multiwrite_merge from creating too large iovecs

[Anthony Liguori]

On 01/19/2010 03:15 PM, Christoph Hellwig wrote:
> If we go over the maximum number of iovecs support by syscall we get
> back EINVAL from the kernel which translate to I/O errors for the guest.
>
> Signed-off-by: Christoph Hellwig<hch@lst.de>
>    

Applied.  Thanks.

Regards,

Anthony Liguori
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c	2010-01-19 22:10:19.797003226 +0100
> +++ qemu/block.c	2010-01-19 22:11:08.226005767 +0100
> @@ -1711,6 +1711,10 @@ static int multiwrite_merge(BlockDriverS
>               merge = bs->drv->bdrv_merge_requests(bs,&reqs[outidx],&reqs[i]);
>           }
>
> +        if (reqs[outidx].qiov->niov + reqs[i].qiov->niov + 1>  IOV_MAX) {
> +            merge = 0;
> +        }
> +
>           if (merge) {
>               size_t size;
>               QEMUIOVector *qiov = qemu_mallocz(sizeof(*qiov));
>
>
>
>    

Re: [Qemu-devel] [PATCH] block: prevent multiwrite_merge from creating too large iovecs

[Christoph Hellwig]

On Wed, Jan 20, 2010 at 12:37:51PM +0100, Kevin Wolf wrote:
> Am 19.01.2010 22:15, schrieb Christoph Hellwig:
> > If we go over the maximum number of iovecs support by syscall we get
> > back EINVAL from the kernel which translate to I/O errors for the guest.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Is this really enough? We don't check for IOV_MAX in any other place, so
> can't we get a too big request directly from virtio-blk?

Currently the virtqueue is limited to 1024 iovecs, but I plan to put in
some better infrastructure to deal with the queue limit.  For now this
patch fixes an issue that we see with real life setups.

Re: [Qemu-devel] [PATCH] block: prevent multiwrite_merge from creating too large iovecs

[Kevin Wolf]

Am 20.01.2010 17:24, schrieb Christoph Hellwig:
> On Wed, Jan 20, 2010 at 12:37:51PM +0100, Kevin Wolf wrote:
>> Am 19.01.2010 22:15, schrieb Christoph Hellwig:
>>> If we go over the maximum number of iovecs support by syscall we get
>>> back EINVAL from the kernel which translate to I/O errors for the guest.
>>>
>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>
>> Is this really enough? We don't check for IOV_MAX in any other place, so
>> can't we get a too big request directly from virtio-blk?
> 
> Currently the virtqueue is limited to 1024 iovecs, but I plan to put in
> some better infrastructure to deal with the queue limit.  For now this
> patch fixes an issue that we see with real life setups.

Ok, if you're planning to replace it by something real, I'm not opposed
to using this as a quick fix for the meantime. However, it needs an
#ifdef for the mingw build breakage at least.

Kevin

Re: [Qemu-devel] [PATCH] block: prevent multiwrite_merge from creating too large iovecs

[Christoph Hellwig]

On Wed, Jan 20, 2010 at 12:37:51PM +0100, Kevin Wolf wrote:
> To underline that it's a backend/platform dependent thing: Your patch
> breaks the mingw build for me.

Actually that's because mingw is the usual piece of crap and doesn't
actually have any of the vector support you can expect from a normal
Unix system.

I can either throw in an #ifdef IOV_MAX around the check or fake one up
for mingw.  Does any of the maintainers have a preference for either
variant?

Re: [Qemu-devel] [PATCH] block: prevent multiwrite_merge from creating too large iovecs

[Anthony Liguori]

On 01/26/2010 03:21 AM, Christoph Hellwig wrote:
> On Wed, Jan 20, 2010 at 12:37:51PM +0100, Kevin Wolf wrote:
>    
>> To underline that it's a backend/platform dependent thing: Your patch
>> breaks the mingw build for me.
>>      
> Actually that's because mingw is the usual piece of crap and doesn't
> actually have any of the vector support you can expect from a normal
> Unix system.
>
> I can either throw in an #ifdef IOV_MAX around the check or fake one up
> for mingw.  Does any of the maintainers have a preference for either
> variant?
>    

grep for CONFIG_IOVEC in qemu-common.h and add a #define IOV_MAX.

mingw doesn't have iovec so we introduce a compat version.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] block: prevent multiwrite_merge from creating too large iovecs

[Christoph Hellwig]

On Tue, Jan 26, 2010 at 07:08:20AM -0600, Anthony Liguori wrote:
> >I can either throw in an #ifdef IOV_MAX around the check or fake one up
> >for mingw.  Does any of the maintainers have a preference for either
> >variant?
> >   
> 
> grep for CONFIG_IOVEC in qemu-common.h and add a #define IOV_MAX.
> 
> mingw doesn't have iovec so we introduce a compat version.

Yes, that's what I meant with the second alternative above.