[PATCH] Use qemu_memalign instead of qemu_malloc

[PATCH] Use qemu_memalign instead of qemu_malloc

[Anthony Liguori]

I guess the main block code is not as defensive as I thought it was.  This patch
uses qemu_memalign to allocate the buffers for IO so that you don't get errors
when using O_DIRECT.

It applies on top of my previous patch to introduce copies in virtio-blk.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/qemu/hw/virtio-blk.c b/qemu/hw/virtio-blk.c
index 2ea5669..669e55f 100644
--- a/qemu/hw/virtio-blk.c
+++ b/qemu/hw/virtio-blk.c
@@ -174,7 +174,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 	    for (i = 1; i < req->elem.out_num; i++)
 		req->size += req->elem.out_sg[i].iov_len;
 
-	    req->buffer = qemu_malloc(req->size);
+	    req->buffer = qemu_memalign(512, req->size);
 	    if (req->buffer == NULL) {
 		qemu_free(req);
 		break;
@@ -203,7 +203,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 	    for (i = 0; i < req->elem.in_num - 1; i++)
 		req->size += req->elem.in_sg[i].iov_len;
 
-	    req->buffer = qemu_malloc(req->size);
+	    req->buffer = qemu_memalign(512, req->size);
 	    if (req->buffer == NULL) {
 		qemu_free(req);
 		break;

Re: [PATCH] Use qemu_memalign instead of qemu_malloc

[Kevin Wolf]

Anthony Liguori schrieb:
> I guess the main block code is not as defensive as I thought it was.  This patch
> uses qemu_memalign to allocate the buffers for IO so that you don't get errors
> when using O_DIRECT.

Actually, the block code should be able to deal with unaligned buffers
since qemu rev. 4599. This change seems to be present in current KVM.
Can you tell exactly which operation failed?

But apart from that, qemu_memalign is the right thing to do, because
copying from/into an aligned buffer in the block code costs performance
(don't know how much, though).

Kevin

Re: [PATCH] Use qemu_memalign instead of qemu_malloc

[Anthony Liguori]

Kevin Wolf wrote:
> Anthony Liguori schrieb:
>   
>> I guess the main block code is not as defensive as I thought it was.  This patch
>> uses qemu_memalign to allocate the buffers for IO so that you don't get errors
>> when using O_DIRECT.
>>     
>
> Actually, the block code should be able to deal with unaligned buffers
> since qemu rev. 4599. This change seems to be present in current KVM.
>   

That was what I thought at first too.

> Can you tell exactly which operation failed?
>   

The aio requests fail with -22 (EINVAL).

> But apart from that, qemu_memalign is the right thing to do, because
> copying from/into an aligned buffer in the block code costs performance
> (don't know how much, though).
>   

Regards,

Anthony Liguori

> Kevin
>   

Re: [PATCH] Use qemu_memalign instead of qemu_malloc

[Kevin Wolf]

Anthony Liguori schrieb:
> Kevin Wolf wrote:
>> Anthony Liguori schrieb:
>>  
>>> I guess the main block code is not as defensive as I thought it was. 
>>> This patch
>>> uses qemu_memalign to allocate the buffers for IO so that you don't
>>> get errors
>>> when using O_DIRECT.
>>>     
>>
>> Actually, the block code should be able to deal with unaligned buffers
>> since qemu rev. 4599. This change seems to be present in current KVM.
>>   
> 
> That was what I thought at first too.
> 
>> Can you tell exactly which operation failed?
> 
> The aio requests fail with -22 (EINVAL).

Yes, if it fails, the EINVAL is no surprise. I meant what code path it
was using. Obviously we missed something in our patch and I'd like to
fix that. Did the error occur on raw images or something like qcow2?

Kevin

Re: [PATCH] Use qemu_memalign instead of qemu_malloc

[Anthony Liguori]

Kevin Wolf wrote:
> Anthony Liguori schrieb:
>   
>> Kevin Wolf wrote:
>>     
>>> Anthony Liguori schrieb:
>>>  
>>>       
>>>> I guess the main block code is not as defensive as I thought it was. 
>>>> This patch
>>>> uses qemu_memalign to allocate the buffers for IO so that you don't
>>>> get errors
>>>> when using O_DIRECT.
>>>>     
>>>>         
>>> Actually, the block code should be able to deal with unaligned buffers
>>> since qemu rev. 4599. This change seems to be present in current KVM.
>>>   
>>>       
>> That was what I thought at first too.
>>
>>     
>>> Can you tell exactly which operation failed?
>>>       
>> The aio requests fail with -22 (EINVAL).
>>     
>
> Yes, if it fails, the EINVAL is no surprise. I meant what code path it
> was using. Obviously we missed something in our patch and I'd like to
> fix that. Did the error occur on raw images or something like qcow2?
>   

It's a raw image and the calls are being made via 
bdrv_aio_read/bdrv_aio_write.  It doesn't occur with a qcow2 but then 
cache=off doesn't seem to do what it's supposed to with cache=off (I 
believe the underlying backing file is not opened O_DIRECT?).

Regards,

Anthony Liguori

> Kevin
>   

Re: [PATCH] Use qemu_memalign instead of qemu_malloc

[Kevin Wolf]

Anthony Liguori schrieb:
> Kevin Wolf wrote:
>> Anthony Liguori schrieb:
>>  
>> Yes, if it fails, the EINVAL is no surprise. I meant what code path it
>> was using. Obviously we missed something in our patch and I'd like to
>> fix that. Did the error occur on raw images or something like qcow2?
>>   
> 
> It's a raw image and the calls are being made via
> bdrv_aio_read/bdrv_aio_write.  It doesn't occur with a qcow2 but then
> cache=off doesn't seem to do what it's supposed to with cache=off (I
> believe the underlying backing file is not opened O_DIRECT?).

This is really strange. In raw_aio_read/write there is a check like this:

    if (unlikely(s->aligned_buf != NULL && ((uintptr_t) buf % 512))) {
        // emulate it using raw_pread/write which uses
        // s->aligned_buf for the request then
    }

For qcow2 I think O_DIRECT actually is in effect. Otherwise it would
have worked even without our patch, and it didn't. And indeed, looking
at the code, it passes flags to bdrv_file_open when it opens the image file.

Kevin

Re: [PATCH] Use qemu_memalign instead of qemu_malloc

[Anthony Liguori]

Kevin Wolf wrote:
> Anthony Liguori schrieb:
>   
>> Kevin Wolf wrote:
>>     
>>> Anthony Liguori schrieb:
>>>  
>>> Yes, if it fails, the EINVAL is no surprise. I meant what code path it
>>> was using. Obviously we missed something in our patch and I'd like to
>>> fix that. Did the error occur on raw images or something like qcow2?
>>>   
>>>       
>> It's a raw image and the calls are being made via
>> bdrv_aio_read/bdrv_aio_write.  It doesn't occur with a qcow2 but then
>> cache=off doesn't seem to do what it's supposed to with cache=off (I
>> believe the underlying backing file is not opened O_DIRECT?).
>>     
>
> This is really strange. In raw_aio_read/write there is a check like this:
>
>     if (unlikely(s->aligned_buf != NULL && ((uintptr_t) buf % 512))) {
>         // emulate it using raw_pread/write which uses
>         // s->aligned_buf for the request then
>     }
>   

Something is goofy then.

> For qcow2 I think O_DIRECT actually is in effect. Otherwise it would
> have worked even without our patch, and it didn't. And indeed, looking
> at the code, it passes flags to bdrv_file_open when it opens the image file.
>   

Something's broken then.  Maybe -snapshot doesn't pick up the 
O_DIRECT'ness?  I'll have to check again.  I was definitely seeing page 
cache behavior with cache=off.

> Kevin
>   

Re: [PATCH] Use qemu_memalign instead of qemu_malloc

[Kevin Wolf]

Anthony Liguori schrieb:
> Kevin Wolf wrote:
>> Anthony Liguori schrieb:
>>  
>>> Kevin Wolf wrote:
>>>    
>>>> Anthony Liguori schrieb:
>>>>  
>>>> Yes, if it fails, the EINVAL is no surprise. I meant what code path it
>>>> was using. Obviously we missed something in our patch and I'd like to
>>>> fix that. Did the error occur on raw images or something like qcow2?
>>>>         
>>> It's a raw image and the calls are being made via
>>> bdrv_aio_read/bdrv_aio_write.  It doesn't occur with a qcow2 but then
>>> cache=off doesn't seem to do what it's supposed to with cache=off (I
>>> believe the underlying backing file is not opened O_DIRECT?).
>>>     
>>
>> This is really strange. In raw_aio_read/write there is a check like this:
>>
>>     if (unlikely(s->aligned_buf != NULL && ((uintptr_t) buf % 512))) {
>>         // emulate it using raw_pread/write which uses
>>         // s->aligned_buf for the request then
>>     }
>>   
> 
> Something is goofy then.
> 
>> For qcow2 I think O_DIRECT actually is in effect. Otherwise it would
>> have worked even without our patch, and it didn't. And indeed, looking
>> at the code, it passes flags to bdrv_file_open when it opens the image
>> file.
>>   
> 
> Something's broken then.  Maybe -snapshot doesn't pick up the
> O_DIRECT'ness?  I'll have to check again.  I was definitely seeing page
> cache behavior with cache=off.

Right, qemu seems to drop the flags for the backing file when using
BDRV_O_SNAPSHOT (bdrv2_open in block.c opens the file). So O_DIRECT
applies only to new data.

Have you been using -snapshot when you had trouble with the unaligned
buffer, too? I don't think I have tested this one when I made the patch...

Kevin

Re: [PATCH] Use qemu_memalign instead of qemu_malloc

[Anthony Liguori]

Kevin Wolf wrote:
> Anthony Liguori schrieb:
>   
>> Kevin Wolf wrote:
>>     
>>> Anthony Liguori schrieb:
>>>  
>>>       
>>>> Kevin Wolf wrote:
>>>>    
>>>>         
>>>>> Anthony Liguori schrieb:
>>>>>  
>>>>> Yes, if it fails, the EINVAL is no surprise. I meant what code path it
>>>>> was using. Obviously we missed something in our patch and I'd like to
>>>>> fix that. Did the error occur on raw images or something like qcow2?
>>>>>         
>>>>>           
>>>> It's a raw image and the calls are being made via
>>>> bdrv_aio_read/bdrv_aio_write.  It doesn't occur with a qcow2 but then
>>>> cache=off doesn't seem to do what it's supposed to with cache=off (I
>>>> believe the underlying backing file is not opened O_DIRECT?).
>>>>     
>>>>         
>>> This is really strange. In raw_aio_read/write there is a check like this:
>>>
>>>     if (unlikely(s->aligned_buf != NULL && ((uintptr_t) buf % 512))) {
>>>         // emulate it using raw_pread/write which uses
>>>         // s->aligned_buf for the request then
>>>     }
>>>   
>>>       
>> Something is goofy then.
>>
>>     
>>> For qcow2 I think O_DIRECT actually is in effect. Otherwise it would
>>> have worked even without our patch, and it didn't. And indeed, looking
>>> at the code, it passes flags to bdrv_file_open when it opens the image
>>> file.
>>>   
>>>       
>> Something's broken then.  Maybe -snapshot doesn't pick up the
>> O_DIRECT'ness?  I'll have to check again.  I was definitely seeing page
>> cache behavior with cache=off.
>>     
>
> Right, qemu seems to drop the flags for the backing file when using
> BDRV_O_SNAPSHOT (bdrv2_open in block.c opens the file). So O_DIRECT
> applies only to new data.
>
> Have you been using -snapshot when you had trouble with the unaligned
> buffer, too? I don't think I have tested this one when I made the patch...
>   

Nope.  I was using a raw image.  Actually, an LVM partition.

Regards,

Anthony Liguori

> Kevin
>   

Re: [PATCH] Use qemu_memalign instead of qemu_malloc

[Laurent Vivier]

Le mercredi 25 juin 2008 à 16:15 +0200, Kevin Wolf a écrit :
> Anthony Liguori schrieb:
> > Kevin Wolf wrote:
> >> Anthony Liguori schrieb:
> >>  
> >>> I guess the main block code is not as defensive as I thought it was. 
> >>> This patch
> >>> uses qemu_memalign to allocate the buffers for IO so that you don't
> >>> get errors
> >>> when using O_DIRECT.
> >>>     
> >>
> >> Actually, the block code should be able to deal with unaligned buffers
> >> since qemu rev. 4599. This change seems to be present in current KVM.
> >>   
> > 
> > That was what I thought at first too.
> > 
> >> Can you tell exactly which operation failed?
> > 
> > The aio requests fail with -22 (EINVAL).
> 
> Yes, if it fails, the EINVAL is no surprise. I meant what code path it
> was using. Obviously we missed something in our patch and I'd like to
> fix that. Did the error occur on raw images or something like qcow2?

Generally EINVAL with O_DIRECT opened files means there is an alignment
problem with offset, buffer address or size to read (must be multiple of
512).

Regards,
Laurent
-- 
------------- Laurent.Vivier@bull.net ---------------
"The best way to predict the future is to invent it."
- Alan Kay

Re: [PATCH] Use qemu_memalign instead of qemu_malloc

[Kevin Wolf]

Laurent Vivier schrieb:
> Generally EINVAL with O_DIRECT opened files means there is an alignment
> problem with offset, buffer address or size to read (must be multiple of
> 512).

Apparently the qemu_memalign for the buffer helps, so it's the buffer
address in this case.

Kevin