[Qemu-devel] [PATCH] QEMU: change default disk cache behavior

[Qemu-devel] [PATCH] QEMU: change default disk cache behavior

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

We seem to get into the discussion of what is the correct default
setting disk images in QEMU. The libvirt team is reluctant to change
specified for newly created images without the default setting
matching it, and everybody seems to agree that the current setting of
WT is the worse possible option.

'nocache' seems to be the preferred option, but it doesn't work for
all cases, like images on ramfs, NFS etc.

Therefore, here is a patch that does two things:
 - default to "nocache"
 - in case of failure with nocache, retry with "write-back"

Jes Sorensen (1):
  QEMU: Change default disk caching to nocache

 vl.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)

[Qemu-devel] [PATCH] QEMU: Change default disk caching to nocache

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Change default disk image caching to nocache (O_DIRECT). However in
case it fails (ramfs, NFS etc.). fall back and retry with write-back.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 vl.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/vl.c b/vl.c
index d77b47c..f3a7d63 100644
--- a/vl.c
+++ b/vl.c
@@ -787,7 +787,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
     int max_devs;
     int index;
     int ro = 0;
-    int bdrv_flags = 0;
+    int bdrv_flags = BDRV_O_NOCACHE;
     int on_read_error, on_write_error;
     const char *devaddr;
     DriveInfo *dinfo;
@@ -910,11 +910,11 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
 
     if ((buf = qemu_opt_get(opts, "cache")) != NULL) {
         if (!strcmp(buf, "off") || !strcmp(buf, "none")) {
-            bdrv_flags |= BDRV_O_NOCACHE;
+            /* default */
         } else if (!strcmp(buf, "writeback")) {
             bdrv_flags |= BDRV_O_CACHE_WB;
         } else if (!strcmp(buf, "writethrough")) {
-            /* this is the default */
+            bdrv_flags &= ~BDRV_O_CACHE_MASK;
         } else {
            fprintf(stderr, "qemu: invalid cache option\n");
            return NULL;
@@ -1120,15 +1120,28 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
     bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
 
     if (bdrv_open(dinfo->bdrv, file, bdrv_flags, drv) < 0) {
-        fprintf(stderr, "qemu: could not open disk image %s: %s\n",
-                        file, strerror(errno));
-        return NULL;
+        if (bdrv_flags & BDRV_O_NOCACHE) {
+            fprintf(stderr, "qemu: failed to open disk image %s as "
+                    "nocache (O_DIRECT) retrying as write-back\n", file);
+            bdrv_flags &= BDRV_O_NOCACHE;
+            bdrv_flags |= BDRV_O_CACHE_WB;
+            if (bdrv_open(dinfo->bdrv, file, bdrv_flags, drv) < 0)
+                goto error_open;
+        } else {
+            goto error_open;
+        }
     }
 
     if (bdrv_key_required(dinfo->bdrv))
         autostart = 0;
     *fatal_error = 0;
     return dinfo;
+
+error_open:
+    fprintf(stderr, "qemu: could not open disk image %s: %s\n",
+            file, strerror(errno));
+    return NULL;
+
 }
 
 static int drive_init_func(QemuOpts *opts, void *opaque)
-- 
1.6.5.2

[Qemu-devel] Re: [PATCH] QEMU: Change default disk caching to nocache

[Paolo Bonzini]

On 05/20/2010 11:32 AM, Jes.Sorensen@redhat.com wrote:
> +        if (bdrv_flags & BDRV_O_NOCACHE) {
> +            fprintf(stderr, "qemu: failed to open disk image %s as "
> +                    "nocache (O_DIRECT) retrying as write-back\n", file);
> +            bdrv_flags &= BDRV_O_NOCACHE;

Missing ~ here.

> +            bdrv_flags |= BDRV_O_CACHE_WB;
> +            if (bdrv_open(dinfo->bdrv, file, bdrv_flags, drv)<  0)
> +                goto error_open;
> +        } else {

I think the retry should be done silently if no cache= option is given. 
  That is cache=none will be the default but:

- if it is not specified and not supported by the image, fall back to 
writeback with no warning.  However, this is just a QoI issue and can be 
fixed later.

- if it is specified and not supported by the image, either fall back to 
writeback with a warning, or fail altogether.  The former would be a 
change in behavior, so it has to be documented somewhere if it changes.

Or maybe add BDRV_O_CACHE_WT and let the backend decide the default?

Paolo

Re: [Qemu-devel] Re: [PATCH] QEMU: Change default disk caching to nocache

[Anthony Liguori]

On 05/20/2010 10:24 AM, Paolo Bonzini wrote:
> On 05/20/2010 11:32 AM, Jes.Sorensen@redhat.com wrote:
>> +        if (bdrv_flags & BDRV_O_NOCACHE) {
>> +            fprintf(stderr, "qemu: failed to open disk image %s as "
>> +                    "nocache (O_DIRECT) retrying as write-back\n", 
>> file);
>> +            bdrv_flags &= BDRV_O_NOCACHE;
>
> Missing ~ here.
>
>> +            bdrv_flags |= BDRV_O_CACHE_WB;
>> +            if (bdrv_open(dinfo->bdrv, file, bdrv_flags, drv)<  0)
>> +                goto error_open;
>> +        } else {
>
> I think the retry should be done silently if no cache= option is 
> given.  That is cache=none will be the default but:
>
> - if it is not specified and not supported by the image, fall back to 
> writeback with no warning.  However, this is just a QoI issue and can 
> be fixed later.
>
> - if it is specified and not supported by the image, either fall back 
> to writeback with a warning, or fail altogether.  The former would be 
> a change in behavior, so it has to be documented somewhere if it changes.
>
> Or maybe add BDRV_O_CACHE_WT and let the backend decide the default?

It used to be that we had a CACHE_DEFAULT which allowed qcow2 to do 
CACHE_WB by default whereas everything else did CACHE_WT.  The same 
technique could be used to let physical devices do NOCACHE by default.

Regards,

Anthony Liguori

> Paolo
>

Re: [Qemu-devel] [PATCH] QEMU: change default disk cache behavior

[Anthony Liguori]

On 05/20/2010 04:32 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> We seem to get into the discussion of what is the correct default
> setting disk images in QEMU. The libvirt team is reluctant to change
> specified for newly created images without the default setting
> matching it, and everybody seems to agree that the current setting of
> WT is the worse possible option.
>
> 'nocache' seems to be the preferred option, but it doesn't work for
> all cases, like images on ramfs, NFS etc.
>
> Therefore, here is a patch that does two things:
>   - default to "nocache"
>   - in case of failure with nocache, retry with "write-back"
>    

This sort of change requires performance data in a variety of 
circumstances to justify.

And I strongly suspect that such a blanket change would be wrong but 
that a more targeted change like making cache=none default for physical 
devices would satisfy mostly everyone.

Regards,

Anthony Liguori

> Jes Sorensen (1):
>    QEMU: Change default disk caching to nocache
>
>   vl.c |   25 +++++++++++++++++++------
>   1 files changed, 19 insertions(+), 6 deletions(-)
>
>
>    

Re: [Qemu-devel] [PATCH] QEMU: change default disk cache behavior

[Jes Sorensen]

On 05/20/10 14:30, Anthony Liguori wrote:
> On 05/20/2010 04:32 AM, Jes.Sorensen@redhat.com wrote:
>> Therefore, here is a patch that does two things:
>>   - default to "nocache"
>>   - in case of failure with nocache, retry with "write-back"
>>    
> 
> This sort of change requires performance data in a variety of
> circumstances to justify.
> 
> And I strongly suspect that such a blanket change would be wrong but
> that a more targeted change like making cache=none default for physical
> devices would satisfy mostly everyone.

Is there any other thing than physical devices attached to the -drive
parameter?

If so, I can take a look at making it more generic when I am back from
holiday next week.

Jes

Re: [Qemu-devel] [PATCH] QEMU: change default disk cache behavior

[Anthony Liguori]

On 05/20/2010 08:36 AM, Jes Sorensen wrote:
> On 05/20/10 14:30, Anthony Liguori wrote:
>    
>> On 05/20/2010 04:32 AM, Jes.Sorensen@redhat.com wrote:
>>      
>>> Therefore, here is a patch that does two things:
>>>    - default to "nocache"
>>>    - in case of failure with nocache, retry with "write-back"
>>>
>>>        
>> This sort of change requires performance data in a variety of
>> circumstances to justify.
>>
>> And I strongly suspect that such a blanket change would be wrong but
>> that a more targeted change like making cache=none default for physical
>> devices would satisfy mostly everyone.
>>      
> Is there any other thing than physical devices attached to the -drive
> parameter?
>    

Image files which are the overwhelming more common use-case.

Regards,

Anthony Liguori

> If so, I can take a look at making it more generic when I am back from
> holiday next week.
>
> Jes
>
>    

Re: [Qemu-devel] [PATCH] QEMU: change default disk cache behavior

[Jes Sorensen]

On 05/20/10 15:40, Anthony Liguori wrote:
> On 05/20/2010 08:36 AM, Jes Sorensen wrote:
>>> And I strongly suspect that such a blanket change would be wrong but
>>> that a more targeted change like making cache=none default for physical
>>> devices would satisfy mostly everyone.
>>>      
>> Is there any other thing than physical devices attached to the -drive
>> parameter?
> 
> Image files which are the overwhelming more common use-case.

For image files we certainly want it too, at least for proper ones (ie.
raw). It could be that it causes problems for qcow2.

I'll try and look at it when I am back.

Cheers,
Jes

Re: [Qemu-devel] [PATCH] QEMU: change default disk cache behavior

[Stefan Hajnoczi]

On Thu, May 20, 2010 at 2:49 PM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
> On 05/20/10 15:40, Anthony Liguori wrote:
>> On 05/20/2010 08:36 AM, Jes Sorensen wrote:
>>>> And I strongly suspect that such a blanket change would be wrong but
>>>> that a more targeted change like making cache=none default for physical
>>>> devices would satisfy mostly everyone.
>>>>
>>> Is there any other thing than physical devices attached to the -drive
>>> parameter?
>>
>> Image files which are the overwhelming more common use-case.
>
> For image files we certainly want it too, at least for proper ones (ie.
> raw). It could be that it causes problems for qcow2.

Qcow2 is safest with cache=writethrough because it doesn't order image updates:

https://bugzilla.redhat.com/show_bug.cgi?id=572825
http://wiki.qemu.org/Features/Qcow2DataIntegrity

Stefan

Re: [Qemu-devel] [PATCH] QEMU: change default disk cache behavior

[Anthony Liguori]

On 05/20/2010 08:49 AM, Jes Sorensen wrote:
> On 05/20/10 15:40, Anthony Liguori wrote:
>    
>> On 05/20/2010 08:36 AM, Jes Sorensen wrote:
>>      
>>>> And I strongly suspect that such a blanket change would be wrong but
>>>> that a more targeted change like making cache=none default for physical
>>>> devices would satisfy mostly everyone.
>>>>
>>>>          
>>> Is there any other thing than physical devices attached to the -drive
>>> parameter?
>>>        
>> Image files which are the overwhelming more common use-case.
>>      
> For image files we certainly want it too, at least for proper ones (ie.
> raw).

What makes you say that?

>   It could be that it causes problems for qcow2.
>    

It's definitely the wrong thing for qcow2 with backing files.

Regards,

Anthony Liguori

> I'll try and look at it when I am back.
>
> Cheers,
> Jes
>
>