[Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Kevin Wolf]

Since the patch that suggested it woudl revert qcow2 to writeback caching, in
fact none of the drivers has had cache=off as default any more. This patch
restores the desired behaviour.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/raw-posix.c |    4 ++++
 block/raw-win32.c |    8 ++++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index fa1a394..513e8e8 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -143,6 +143,10 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
 
     /* Use O_DSYNC for write-through caching, no flags for write-back caching,
      * and O_DIRECT for no caching. */
+    if (bdrv_flags & BDRV_O_CACHE_DEF) {
+        bdrv_flags = (bdrv_flags & ~BDRV_O_CACHE_MASK) | BDRV_O_NOCACHE;
+    }
+
     if ((bdrv_flags & BDRV_O_NOCACHE))
         s->open_flags |= O_DIRECT;
     else if (!(bdrv_flags & BDRV_O_CACHE_WB))
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 72acad5..5da957c 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -92,6 +92,10 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags)
         create_flags = OPEN_EXISTING;
     }
     overlapped = FILE_ATTRIBUTE_NORMAL;
+
+    if (flags & BDRV_O_CACHE_DEF) {
+        flags = (flags & ~BDRV_O_CACHE_MASK) | BDRV_O_NOCACHE;
+    }
     if ((flags & BDRV_O_NOCACHE))
         overlapped |= FILE_FLAG_NO_BUFFERING | FILE_FLAG_WRITE_THROUGH;
     else if (!(flags & BDRV_O_CACHE_WB))
@@ -345,6 +349,10 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
     create_flags = OPEN_EXISTING;
 
     overlapped = FILE_ATTRIBUTE_NORMAL;
+
+    if (flags & BDRV_O_CACHE_DEF) {
+        flags = (flags & ~BDRV_O_CACHE_MASK) | BDRV_O_NOCACHE;
+    }
     if ((flags & BDRV_O_NOCACHE))
         overlapped |= FILE_FLAG_NO_BUFFERING | FILE_FLAG_WRITE_THROUGH;
     else if (!(flags & BDRV_O_CACHE_WB))
-- 
1.6.0.6

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Christoph Hellwig]

On Mon, Jun 22, 2009 at 01:18:03PM +0200, Kevin Wolf wrote:
> Since the patch that suggested it woudl revert qcow2 to writeback caching, in
> fact none of the drivers has had cache=off as default any more. This patch
> restores the desired behaviour.

Looks good to me.  I still quite hate that BDRV_O_CACHE_DEF thing, just
makes it too complicated for drivers to get it right.

Btw, what is the performance for qcow2 without write-back caching with
all your improvements?

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Kevin Wolf]

Christoph Hellwig schrieb:
> On Mon, Jun 22, 2009 at 01:18:03PM +0200, Kevin Wolf wrote:
>> Since the patch that suggested it woudl revert qcow2 to writeback caching, in
>> fact none of the drivers has had cache=off as default any more. This patch
>> restores the desired behaviour.
> 
> Looks good to me.  I still quite hate that BDRV_O_CACHE_DEF thing, just
> makes it too complicated for drivers to get it right.

Actually it's just raw that must get it right. All the other drivers
access their image file through raw.

> Btw, what is the performance for qcow2 without write-back caching with
> all your improvements?

I haven't done real benchmarks for some time, but my simple test cases
like "run mke2fs on an empty image" have shown great improvement (at
least on IDE, for virtio I haven't understood yet what influences the
size of its requests - you should have received a private mail from Dor
about this). Possibly we could even enable cache=off for qcow2 now.

IIUC, you have a nice setup to measure block performance now. Maybe
you'd like to let it run with qcow2 and cache=wb/none?

Kevin

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Avi Kivity]

On 06/22/2009 02:44 PM, Kevin Wolf wrote:
> I haven't done real benchmarks for some time, but my simple test cases
> like "run mke2fs on an empty image" have shown great improvement (at
> least on IDE, for virtio I haven't understood yet what influences the
> size of its requests - you should have received a private mail from Dor
> about this). Possibly we could even enable cache=off for qcow2 now.
>
> IIUC, you have a nice setup to measure block performance now. Maybe
> you'd like to let it run with qcow2 and cache=wb/none?
>    

Were your refcount-combining patches merged?  I don't see them, and 
performance will suck without them.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Kevin Wolf]

Avi Kivity schrieb:
> On 06/22/2009 02:44 PM, Kevin Wolf wrote:
>> I haven't done real benchmarks for some time, but my simple test cases
>> like "run mke2fs on an empty image" have shown great improvement (at
>> least on IDE, for virtio I haven't understood yet what influences the
>> size of its requests - you should have received a private mail from Dor
>> about this). Possibly we could even enable cache=off for qcow2 now.
>>
>> IIUC, you have a nice setup to measure block performance now. Maybe
>> you'd like to let it run with qcow2 and cache=wb/none?
>>    
> 
> Were your refcount-combining patches merged?  I don't see them, and 
> performance will suck without them.

They were merged some weeks ago, at least if a980c98c is what you mean.
I don't have any patches for inclusion that aren't merged yet (except
for this one, obviously).

Kevin

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Avi Kivity]

On 06/22/2009 03:32 PM, Kevin Wolf wrote:
>> Were your refcount-combining patches merged?  I don't see them, and
>> performance will suck without them.
>>      
>
> They were merged some weeks ago, at least if a980c98c is what you mean.
> I don't have any patches for inclusion that aren't merged yet (except
> for this one, obviously).
>    

I meant that, yes.  Missed it going in.

We still have a read-modify-write when extending an image, but I guess 
we're pretty close now, so it's worthwhile to try a guest install with 
cache=off.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Kevin Wolf]

Avi Kivity schrieb:
> On 06/22/2009 03:32 PM, Kevin Wolf wrote:
>>> Were your refcount-combining patches merged?  I don't see them, and
>>> performance will suck without them.
>>>      
>> They were merged some weeks ago, at least if a980c98c is what you mean.
>> I don't have any patches for inclusion that aren't merged yet (except
>> for this one, obviously).
>>    
> 
> I meant that, yes.  Missed it going in.
> 
> We still have a read-modify-write when extending an image, but I guess 
> we're pretty close now, so it's worthwhile to try a guest install with 
> cache=off.

As long as we don't have overlapping requests the RMW is basically a
memset, so at least on IDE this shouldn't hurt too much.

What happens with virtio I still need to understand. Obviously, as soon
as virtio decides to fall back to 4k requests, performance becomes
terrible. And if it doesn't, we still can get concurrent requests to the
same cluster, resulting in a real RMW (however, if it's only one RMW
remaining, it's kind of okay - last week's patches have removed some
more of them in the cluster allocation path...)

Kevin

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Christoph Hellwig]

On Mon, Jun 22, 2009 at 01:44:35PM +0200, Kevin Wolf wrote:
> IIUC, you have a nice setup to measure block performance now. Maybe
> you'd like to let it run with qcow2 and cache=wb/none?

The setup is not quite nice yet, still involves a lot of manual setup.
I'm working on recovering my shell script super powers to automate it
and then we can throw all kinds of tests at it without much work.

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Kevin Wolf]

Christoph Hellwig schrieb:
> On Mon, Jun 22, 2009 at 01:44:35PM +0200, Kevin Wolf wrote:
>> IIUC, you have a nice setup to measure block performance now. Maybe
>> you'd like to let it run with qcow2 and cache=wb/none?
> 
> The setup is not quite nice yet, still involves a lot of manual setup.
> I'm working on recovering my shell script super powers to automate it
> and then we can throw all kinds of tests at it without much work.

Ok. Never mind then.

Kevin

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Anthony Liguori]

Kevin Wolf wrote:
> Since the patch that suggested it woudl revert qcow2 to writeback caching, in
> fact none of the drivers has had cache=off as default any more. This patch
> restores the desired behaviour.
>   

When has cache=off ever been the default?

Moreover, why would we want to make it the default?  A patch with this 
drastic of a change really need a more significant justification in the 
change log.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Avi Kivity]

On 06/22/2009 04:04 PM, Christoph Hellwig wrote:
> On Mon, Jun 22, 2009 at 01:44:35PM +0200, Kevin Wolf wrote:
>    
>> IIUC, you have a nice setup to measure block performance now. Maybe
>> you'd like to let it run with qcow2 and cache=wb/none?
>>      
>
> The setup is not quite nice yet, still involves a lot of manual setup.
> I'm working on recovering my shell script super powers to automate it
> and then we can throw all kinds of tests at it without much work.
>    

Shell gets very nasty very fast.  Can I recommend Python?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Kevin Wolf]

Anthony Liguori schrieb:
> Kevin Wolf wrote:
>> Since the patch that suggested it woudl revert qcow2 to writeback caching, in
>> fact none of the drivers has had cache=off as default any more. This patch
>> restores the desired behaviour.
>>   
> 
> When has cache=off ever been the default?
> 
> Moreover, why would we want to make it the default?  A patch with this 
> drastic of a change really need a more significant justification in the 
> change log.

Hm, you're right. Why is everyone talking about cache=off being default
except for qcow2? This was confusing me. Not handling the default
explicitly but checking the negation of it in 'else if (!(bdrv_flags &
BDRV_O_CACHE_WB))' has done the rest.

Sorry, I take this patch back.

Kevin

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Dor Laor]

On 06/22/2009 04:08 PM, Anthony Liguori wrote:
> Kevin Wolf wrote:
>> Since the patch that suggested it woudl revert qcow2 to writeback 
>> caching, in
>> fact none of the drivers has had cache=off as default any more. This 
>> patch
>> restores the desired behaviour.
>
> When has cache=off ever been the default?
>
> Moreover, why would we want to make it the default?  A patch with this 
> drastic of a change really need a more 

It should be the default (or at least O_DSYNC) since plain users are not 
aware that writeback might
cause their image to get corrupted on host crash.

> significant justification in the change log.
>
> Regards,
>
> Anthony Liguori
>
>

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Kevin Wolf]

Dor Laor schrieb:
> On 06/22/2009 04:08 PM, Anthony Liguori wrote:
>> Kevin Wolf wrote:
>>> Since the patch that suggested it woudl revert qcow2 to writeback 
>>> caching, in
>>> fact none of the drivers has had cache=off as default any more. This 
>>> patch
>>> restores the desired behaviour.
>> When has cache=off ever been the default?
>>
>> Moreover, why would we want to make it the default?  A patch with this 
>> drastic of a change really need a more 
> 
> It should be the default (or at least O_DSYNC) since plain users are not 
> aware that writeback might
> cause their image to get corrupted on host crash.

Anthony is right, I was confused. writeback isn't the default, it's
writethrough and it actually works. The code is just not explicit enough
when you're convinced that caching is meant be off by default because
this is what everyone is talking about.

Kevin

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Anthony Liguori]

Kevin Wolf wrote:
> Anthony Liguori schrieb:
>   
>> Kevin Wolf wrote:
>>     
>>> Since the patch that suggested it woudl revert qcow2 to writeback caching, in
>>> fact none of the drivers has had cache=off as default any more. This patch
>>> restores the desired behaviour.
>>>   
>>>       
>> When has cache=off ever been the default?
>>
>> Moreover, why would we want to make it the default?  A patch with this 
>> drastic of a change really need a more significant justification in the 
>> change log.
>>     
>
> Hm, you're right. Why is everyone talking about cache=off being default
> except for qcow2? This was confusing me.

qcow2 defaults to writeback whereas everything else defaults to 
writethrough.

So probably taking your patch and s/NOCACHE/CACHE_WT/g would be pretty 
reasonable :-)

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Jamie Lokier]

Kevin Wolf wrote:
> What happens with virtio I still need to understand. Obviously, as soon
> as virtio decides to fall back to 4k requests, performance becomes
> terrible.

Does emulating a disk with 4k sector size instead of 512 bytes help this?

-- Jamie

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Kevin Wolf]

Jamie Lokier schrieb:
> Kevin Wolf wrote:
>> What happens with virtio I still need to understand. Obviously, as soon
>> as virtio decides to fall back to 4k requests, performance becomes
>> terrible.
> 
> Does emulating a disk with 4k sector size instead of 512 bytes help this?

I just changed the virtio_blk code to always do the
blk_queue_hardsect_size with 4096, didn't change the behaviour.

I'm not sure if I have mentioned it in this thread: We have found that
it helps to use the deadline elevator instead of cfq in either the host
or the guest. I would accept this if it would only help when it's
changed in the guest (after all, I don't know the Linux block layer very
well), but I certainly don't understand how the host elevator could
change the guest request sizes - and noone else on the internal mailing
lists had an explanation either.

Kevin

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Jamie Lokier]

Kevin Wolf wrote:
> Jamie Lokier schrieb:
> > Kevin Wolf wrote:
> >> What happens with virtio I still need to understand. Obviously, as soon
> >> as virtio decides to fall back to 4k requests, performance becomes
> >> terrible.
> > 
> > Does emulating a disk with 4k sector size instead of 512 bytes help this?
> 
> I just changed the virtio_blk code to always do the
> blk_queue_hardsect_size with 4096, didn't change the behaviour.

You need quite a bit more than that to emulate a 4k sector size disk.
There's the ATA/SCSI ID pages to update, and the special 512-bit
offset tricky thing.

> I'm not sure if I have mentioned it in this thread: We have found that
> it helps to use the deadline elevator instead of cfq in either the host
> or the guest. I would accept this if it would only help when it's
> changed in the guest (after all, I don't know the Linux block layer very
> well), but I certainly don't understand how the host elevator could
> change the guest request sizes - and noone else on the internal mailing
> lists had an explanation either.

The host elevator will certainly affect the timing of I/O requests,
which it receives from the guest, and it will also affect how requests
are merged to make larger requests.

So it's not surprising that the host elevator changes the sizes of
request sizes when they reach the host disk.

It shouldn't change the size of requests inside the guest, _before_
they reach the host.

-- Jamie

Re: [Qemu-devel] [PATCH] block-raw: Make cache=off default again

[Kevin Wolf]

Jamie Lokier schrieb:
> Kevin Wolf wrote:
>> Jamie Lokier schrieb:
>>> Kevin Wolf wrote:
>>>> What happens with virtio I still need to understand. Obviously, as soon
>>>> as virtio decides to fall back to 4k requests, performance becomes
>>>> terrible.
>>> Does emulating a disk with 4k sector size instead of 512 bytes help this?
>> I just changed the virtio_blk code to always do the
>> blk_queue_hardsect_size with 4096, didn't change the behaviour.
> 
> You need quite a bit more than that to emulate a 4k sector size disk.
> There's the ATA/SCSI ID pages to update, and the special 512-bit
> offset tricky thing.

Okay, then I'll just admit that I know too little about the Linux block
layer. I'll gladly try any patch (and hopefully understand it then), but
for doing it myself I think it would take me a lot of time experimenting
and still not knowing if I'm doing it right.

>> I'm not sure if I have mentioned it in this thread: We have found that
>> it helps to use the deadline elevator instead of cfq in either the host
>> or the guest. I would accept this if it would only help when it's
>> changed in the guest (after all, I don't know the Linux block layer very
>> well), but I certainly don't understand how the host elevator could
>> change the guest request sizes - and noone else on the internal mailing
>> lists had an explanation either.
> 
> The host elevator will certainly affect the timing of I/O requests,
> which it receives from the guest, and it will also affect how requests
> are merged to make larger requests.
> 
> So it's not surprising that the host elevator changes the sizes of
> request sizes when they reach the host disk.
> 
> It shouldn't change the size of requests inside the guest, _before_
> they reach the host.

Yeah, this is exactly what I was thinking, too. However, in reality it
_does_ influence the guest request sizes, for whatever reason (maybe
again something timing related?). I put debug code into both the qemu
virtio-blk implementation and the guest kernel module and they both see
lots of 4k requests when the host uses cfg and much larger requests when
it uses deadline.

Kevin