Re: [PATCH] Use qemu_memalign instead of qemu_malloc
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use qemu_memalign instead of qemu_malloc
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use qemu_memalign instead of qemu_malloc
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use qemu_memalign instead of qemu_malloc
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use qemu_memalign instead of qemu_malloc
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 -- - [EMAIL PROTECTED] --- "The best way to predict the future is to invent it." - Alan Kay -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use qemu_memalign instead of qemu_malloc
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use qemu_memalign instead of qemu_malloc
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use qemu_memalign instead of qemu_malloc
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use qemu_memalign instead of qemu_malloc
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use qemu_memalign instead of qemu_malloc
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Use qemu_memalign instead of qemu_malloc
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 <[EMAIL PROTECTED]>
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;
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
