[PATCH 0/3] qemu-xen-traditional: disk performance improvements

[PATCH 0/3] qemu-xen-traditional: disk performance improvements

[Stefano Stabellini]

Hi all,
this small patch series enables the O_DIRECT flag to open disk images
for both the IDE and xen_disk interface.
Also it includes few fixes for xen_disk.

Stefano Stabellini (3):
      qemu-xen-traditional: use O_DIRECT to open disk images for IDE
      qemu-xen-traditional: use O_DIRECT to open disk images with QDISK
      qemu-xen-traditional: QDISK fixes

 hw/xen_disk.c |   17 +++++++++--------
 xenstore.c    |    2 +-
 2 files changed, 10 insertions(+), 9 deletions(-)

Cheers,

Stefano

[PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xenstore.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/xenstore.c b/xenstore.c
index 4c483e2..ac90366 100644
--- a/xenstore.c
+++ b/xenstore.c
@@ -643,7 +643,7 @@ void xenstore_parse_domain_config(int hvm_domid)
 	    }
             pstrcpy(bs->filename, sizeof(bs->filename), params);
 
-            flags = BDRV_O_CACHE_WB; /* snapshot and write-back */
+            flags = BDRV_O_NOCACHE;
             is_readonly = 0;
             if (pasprintf(&buf, "%s/mode", bpath) == -1)
                 continue;
-- 
1.7.2.5

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE

[Zhang, Yang Z]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org
> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Stefano Stabellini
> Sent: Friday, March 23, 2012 10:51 PM
> To: xen-devel@lists.xensource.com
> Cc: Ian.Jackson@eu.citrix.com; Stefano Stabellini
> Subject: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open
> disk images for IDE
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xenstore.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/xenstore.c b/xenstore.c
> index 4c483e2..ac90366 100644
> --- a/xenstore.c
> +++ b/xenstore.c
> @@ -643,7 +643,7 @@ void xenstore_parse_domain_config(int hvm_domid)
>  	    }
>              pstrcpy(bs->filename, sizeof(bs->filename), params);
> 
> -            flags = BDRV_O_CACHE_WB; /* snapshot and write-back */
> +            flags = BDRV_O_NOCACHE;
>              is_readonly = 0;
>              if (pasprintf(&buf, "%s/mode", bpath) == -1)
>                  continue;
Any reason for this change? 

best regards
yang

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE

[Stefano Stabellini]

On Mon, 26 Mar 2012, Zhang, Yang Z wrote:
> > -----Original Message-----
> > From: xen-devel-bounces@lists.xen.org
> > [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Stefano Stabellini
> > Sent: Friday, March 23, 2012 10:51 PM
> > To: xen-devel@lists.xensource.com
> > Cc: Ian.Jackson@eu.citrix.com; Stefano Stabellini
> > Subject: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open
> > disk images for IDE
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  xenstore.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/xenstore.c b/xenstore.c
> > index 4c483e2..ac90366 100644
> > --- a/xenstore.c
> > +++ b/xenstore.c
> > @@ -643,7 +643,7 @@ void xenstore_parse_domain_config(int hvm_domid)
> >  	    }
> >              pstrcpy(bs->filename, sizeof(bs->filename), params);
> > 
> > -            flags = BDRV_O_CACHE_WB; /* snapshot and write-back */
> > +            flags = BDRV_O_NOCACHE;
> >              is_readonly = 0;
> >              if (pasprintf(&buf, "%s/mode", bpath) == -1)
> >                  continue;
> Any reason for this change? 

performance and correctness: BDRV_O_NOCACHE in QEMU means opening the
disk image with O_DIRECT

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE

[Zhang, Yang Z]

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Monday, March 26, 2012 6:21 PM
> To: Zhang, Yang Z
> Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Ian Jackson
> Subject: RE: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to
> open disk images for IDE
> 
> On Mon, 26 Mar 2012, Zhang, Yang Z wrote:
> > > -----Original Message-----
> > > From: xen-devel-bounces@lists.xen.org
> > > [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Stefano Stabellini
> > > Sent: Friday, March 23, 2012 10:51 PM
> > > To: xen-devel@lists.xensource.com
> > > Cc: Ian.Jackson@eu.citrix.com; Stefano Stabellini
> > > Subject: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to
> open
> > > disk images for IDE
> > >
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > ---
> > >  xenstore.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/xenstore.c b/xenstore.c
> > > index 4c483e2..ac90366 100644
> > > --- a/xenstore.c
> > > +++ b/xenstore.c
> > > @@ -643,7 +643,7 @@ void xenstore_parse_domain_config(int hvm_domid)
> > >  	    }
> > >              pstrcpy(bs->filename, sizeof(bs->filename), params);
> > >
> > > -            flags = BDRV_O_CACHE_WB; /* snapshot and write-back */
> > > +            flags = BDRV_O_NOCACHE;
> > >              is_readonly = 0;
> > >              if (pasprintf(&buf, "%s/mode", bpath) == -1)
> > >                  continue;
> > Any reason for this change?
> 
> performance and correctness: BDRV_O_NOCACHE in QEMU means opening the
> disk image with O_DIRECT
Doesn't cache mode have better performance than NOCACHE?

best regards
yang

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE

[Stefano Stabellini]

On Tue, 27 Mar 2012, Zhang, Yang Z wrote:
> > > > diff --git a/xenstore.c b/xenstore.c
> > > > index 4c483e2..ac90366 100644
> > > > --- a/xenstore.c
> > > > +++ b/xenstore.c
> > > > @@ -643,7 +643,7 @@ void xenstore_parse_domain_config(int hvm_domid)
> > > >  	    }
> > > >              pstrcpy(bs->filename, sizeof(bs->filename), params);
> > > >
> > > > -            flags = BDRV_O_CACHE_WB; /* snapshot and write-back */
> > > > +            flags = BDRV_O_NOCACHE;
> > > >              is_readonly = 0;
> > > >              if (pasprintf(&buf, "%s/mode", bpath) == -1)
> > > >                  continue;
> > > Any reason for this change?
> > 
> > performance and correctness: BDRV_O_NOCACHE in QEMU means opening the
> > disk image with O_DIRECT
> Doesn't cache mode have better performance than NOCACHE?

Actually you are correct. I think that this patch should be dropped from
the series. Of course we need O_DIRECT for QDISK otherwise we do loose
correctness but considering that IDE should only be used during
installation it can stay as it is.

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE

[Ian Jackson]

Stefano Stabellini writes ("RE: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE"):
> On Tue, 27 Mar 2012, Zhang, Yang Z wrote:
> > Doesn't cache mode have better performance than NOCACHE?
> 
> Actually you are correct. I think that this patch should be dropped from
> the series. Of course we need O_DIRECT for QDISK otherwise we do loose
> correctness but considering that IDE should only be used during
> installation it can stay as it is.

I don't think this assumption about IDE is correct.  To say that "IDE
should only be used during installation" is not an excuse for
providing an IDE controller which violates the usual correctness
rules.

Ian.

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE

[Joseph Glanville]

On 28 March 2012 04:22, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Stefano Stabellini writes ("RE: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE"):
>> On Tue, 27 Mar 2012, Zhang, Yang Z wrote:
>> > Doesn't cache mode have better performance than NOCACHE?
>>
>> Actually you are correct. I think that this patch should be dropped from
>> the series. Of course we need O_DIRECT for QDISK otherwise we do loose
>> correctness but considering that IDE should only be used during
>> installation it can stay as it is.
>
> I don't think this assumption about IDE is correct.  To say that "IDE
> should only be used during installation" is not an excuse for
> providing an IDE controller which violates the usual correctness
> rules.
>
> Ian.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

I would highly recommend using direct IO where virtualized disks are
concerned, regardless of correctness.
At the very least based on the principle of "least surprise" noone
likes to find out their data was corrupted due to caches they didn't
know about and didn't configure.
Over the years this has been well documented in the Xen archives and
old wiki as unwanted and best avoided behavior.

This is the driving force behind persuing qdisk over blkback+loop
until loop attains O_DIRECT support upstream.

The other option is to document that this is the case and recommend
users always use the phy:/ handler with proper block devices.
Though IMHO this is not desired and it would be preffered to indicate
that use of file:/ will result in performance degradation.

Joseph.

-- 
Founder | Director | VP Research
Orion Virtualisation Solutions | www.orionvm.com.au | Phone: 1300 56
99 52 | Mobile: 0428 754 846

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE

[Ian Campbell]

On Tue, 2012-03-27 at 18:22 +0100, Ian Jackson wrote:
> Stefano Stabellini writes ("RE: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE"):
> > On Tue, 27 Mar 2012, Zhang, Yang Z wrote:
> > > Doesn't cache mode have better performance than NOCACHE?
> > 
> > Actually you are correct. I think that this patch should be dropped from
> > the series. Of course we need O_DIRECT for QDISK otherwise we do loose
> > correctness but considering that IDE should only be used during
> > installation it can stay as it is.
> 
> I don't think this assumption about IDE is correct.  To say that "IDE
> should only be used during installation" is not an excuse for
> providing an IDE controller which violates the usual correctness
> rules.

The changeset which originally made this use BDRV_O_CACHE is below, do
the arguments made there no longer apply? To my non-qemu eye it looks
like hw/ide.c is doing an appropriate amount of bdrv_flush().

I think it is possible that we've incorrectly determined that
BDRV_O_CACHE has issues with correctness?

My recollection is that way-back-when that installation to an emulated
IDE device with O_DIRECT was so slow that it was deemed an acceptable
trade-off, presumably given the understanding that IDE cache control was
working.

I think Stefano measured it again recently, Stefano -- can you share the
numbers you saw?

Ian.

commit 82787c6f689d869ad349df83ec3f58702afe00fe
Author: Ian Jackson <ian.jackson@eu.citrix.com>
Date:   Mon Mar 2 11:21:51 2009 +0000

    Override default cache mode for disk images to write-back
    
    Upstream qemu changed the default cache mode to write-through (ie,
    O_DSYNC) which is much slower.  We do not need this as we have
    explicit control of cacheing with the IDE cache control commands.
    
    Original patch by Yang Zhang modified by Ian Jackson.
    
    Signed-off-by: Yang Zhang <yang.zhang@intel.com>
    Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

diff --git a/xenstore.c b/xenstore.c
index 6bfcdbb..928e950 100644
--- a/xenstore.c
+++ b/xenstore.c
@@ -472,7 +472,7 @@ void xenstore_parse_domain_config(int hvm_domid)
 #ifdef CONFIG_STUBDOM
         if (pasprintf(&danger_buf, "%s/device/vbd/%s", danger_path, e_danger[i]
             continue;
-       if (bdrv_open2(bs, danger_buf, 0 /* snapshot */, &bdrv_vbd) == 0) {
+       if (bdrv_open2(bs, danger_buf, BDRV_O_CACHE_WB /* snapshot and write-bac
            pstrcpy(bs->filename, sizeof(bs->filename), params);
        } else
 #endif
@@ -498,7 +498,7 @@ void xenstore_parse_domain_config(int hvm_domid)
                }
            }
             pstrcpy(bs->filename, sizeof(bs->filename), params);
-            if (bdrv_open2(bs, params, 0 /* snapshot */, format) < 0)
+            if (bdrv_open2(bs, params, BDRV_O_CACHE_WB /* snapshot and write-ba
                 fprintf(stderr, "qemu: could not open vbd '%s' or hard disk ima
         }
 

> 
> Ian.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE

[Stefano Stabellini]

On Wed, 28 Mar 2012, Ian Campbell wrote:
> On Tue, 2012-03-27 at 18:22 +0100, Ian Jackson wrote:
> > Stefano Stabellini writes ("RE: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE"):
> > > On Tue, 27 Mar 2012, Zhang, Yang Z wrote:
> > > > Doesn't cache mode have better performance than NOCACHE?
> > > 
> > > Actually you are correct. I think that this patch should be dropped from
> > > the series. Of course we need O_DIRECT for QDISK otherwise we do loose
> > > correctness but considering that IDE should only be used during
> > > installation it can stay as it is.
> > 
> > I don't think this assumption about IDE is correct.  To say that "IDE
> > should only be used during installation" is not an excuse for
> > providing an IDE controller which violates the usual correctness
> > rules.
> 
> The changeset which originally made this use BDRV_O_CACHE is below, do
> the arguments made there no longer apply? To my non-qemu eye it looks
> like hw/ide.c is doing an appropriate amount of bdrv_flush().

It is not just about the IDE controller, it is also about the image
format, see below.


> I think it is possible that we've incorrectly determined that
> BDRV_O_CACHE has issues with correctness?

Following the latest disk cache thread on qemu-devel
(http://marc.info/?l=qemu-devel&m=127434799425483) it looks like some
image formats are unsafe with BDRV_O_CACHE_WB:

https://bugzilla.redhat.com/show_bug.cgi?id=572825

It looks like KVM suggests to turn off caching with raw files or
volumes (but keep in mind that the default for them is write through):

http://www.linux-kvm.org/page/Tuning_KVM


> My recollection is that way-back-when that installation to an emulated
> IDE device with O_DIRECT was so slow that it was deemed an acceptable
> trade-off, presumably given the understanding that IDE cache control was
> working.
> 
> I think Stefano measured it again recently, Stefano -- can you share the
> numbers you saw?

Installing Win7 X64 on my testbox takes 15 minutes with BDRV_O_CACHE_WB
and 23 minutes with BDRV_O_NOCACHE.
That means a 35% drop in performances.
Given the high difference I would be tempted to keep things as they are,
at least for raw files/devices and maybe switch to nocache just for cow
formats.

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE [and 1 more messages]

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE"):
> On Thu, 29 Mar 2012, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE"):
> > So I think qemu-xen-unstable the use of write-back cacheing for
> > bdrv_write is correct.  (Apart from the wrinkle that the emulated
> > drive cache, write_cache, is enabled by default - but I wouldn't be
> > surprised if that accurately emulated the behaviour of real disks.)
> 
> You have missed my other emails on this thread about other disk formats
> and related bug reports.

I guess you mean this, which I had read:

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE"):
> On Wed, 28 Mar 2012, Ian Campbell wrote:
> > I think it is possible that we've incorrectly determined that
> > BDRV_O_CACHE has issues with correctness?
> 
> Following the latest disk cache thread on qemu-devel
> (http://marc.info/?l=qemu-devel&m=127434799425483) it looks like some
> image formats are unsafe with BDRV_O_CACHE_WB:

I'm not convinced that this bug applies to the approach taken in
qemu-xen-unstable, because in qemu-xen-unstable we always call
bdrv_flush at an appropriate point, and bdrv_flush is plumbed through
by qcow_aio_flush to a flush of the underlying device.

Ian.

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE [and 1 more messages]

[Stefano Stabellini]

On Thu, 29 Mar 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE"):
> > On Thu, 29 Mar 2012, Ian Jackson wrote:
> > > Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE"):
> > > So I think qemu-xen-unstable the use of write-back cacheing for
> > > bdrv_write is correct.  (Apart from the wrinkle that the emulated
> > > drive cache, write_cache, is enabled by default - but I wouldn't be
> > > surprised if that accurately emulated the behaviour of real disks.)
> > 
> > You have missed my other emails on this thread about other disk formats
> > and related bug reports.
> 
> I guess you mean this, which I had read:
> 
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE"):
> > On Wed, 28 Mar 2012, Ian Campbell wrote:
> > > I think it is possible that we've incorrectly determined that
> > > BDRV_O_CACHE has issues with correctness?
> > 
> > Following the latest disk cache thread on qemu-devel
> > (http://marc.info/?l=qemu-devel&m=127434799425483) it looks like some
> > image formats are unsafe with BDRV_O_CACHE_WB:
> 
> I'm not convinced that this bug applies to the approach taken in
> qemu-xen-unstable, because in qemu-xen-unstable we always call
> bdrv_flush at an appropriate point, and bdrv_flush is plumbed through
> by qcow_aio_flush to a flush of the underlying device.
 
I think it does.
Here is an example:


In qemu-xen you can easily have this sequence of calls:

bdrv_co_writev -> qcow2_co_writev -> qcow2_alloc_cluster_offset ->
get_cluster_table -> qcow2_grow_l1_table -> bdrv_pwrite_sync



In qemu-xen-traditional they would become:

qcow_aio_write_cb -> alloc_cluster_offset -> get_cluster_table ->
grow_l1_table -> bdrv_pwrite


The key piece of information is that bdrv_pwrite_sync issues a
bdrv_flush if the file was opened as WB, while bdrv_pwrite does not.

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE [and 1 more messages]

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE [and 1 more messages]"):
> In qemu-xen you can easily have this sequence of calls:
> 
> bdrv_co_writev -> qcow2_co_writev -> qcow2_alloc_cluster_offset ->
> get_cluster_table -> qcow2_grow_l1_table -> bdrv_pwrite_sync
...
> In qemu-xen-traditional they would become:
> 
> qcow_aio_write_cb -> alloc_cluster_offset -> get_cluster_table ->
> grow_l1_table -> bdrv_pwrite
> 
> 
> The key piece of information is that bdrv_pwrite_sync issues a
> bdrv_flush if the file was opened as WB, while bdrv_pwrite does not.

Yes, but why does this matter ?  If the guest cares about the data
then it will either turn off caching entirely, or issue an explicit
flush operation, which will translate into a bdrv_flush.  So the flush
will happen.

Ian.

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE [and 1 more messages]

[Stefano Stabellini]

On Fri, 30 Mar 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE [and 1 more messages]"):
> > In qemu-xen you can easily have this sequence of calls:
> > 
> > bdrv_co_writev -> qcow2_co_writev -> qcow2_alloc_cluster_offset ->
> > get_cluster_table -> qcow2_grow_l1_table -> bdrv_pwrite_sync
> ...
> > In qemu-xen-traditional they would become:
> > 
> > qcow_aio_write_cb -> alloc_cluster_offset -> get_cluster_table ->
> > grow_l1_table -> bdrv_pwrite
> > 
> > 
> > The key piece of information is that bdrv_pwrite_sync issues a
> > bdrv_flush if the file was opened as WB, while bdrv_pwrite does not.
> 
> Yes, but why does this matter ?  If the guest cares about the data
> then it will either turn off caching entirely, or issue an explicit
> flush operation, which will translate into a bdrv_flush.  So the flush
> will happen.
 
If the QCOW2 metadata is written/updated in response to events other
than bdrv_write, they could still get out of sync.

However giving a look at block-qcow2.c I wasn't able to find any such
cases (aside snapshots that we don't support), so it should be OK to use
WB with IDE and QCOW2 on qemu-xen-traditional too.

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE

[Zhang, Yang Z]

> -----Original Message-----
> From: Ian Campbell [mailto:Ian.Campbell@citrix.com]
> Sent: Wednesday, March 28, 2012 5:11 PM
> To: Ian Jackson
> Cc: Stefano Stabellini; Zhang, Yang Z; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to
> open disk images for IDE
> 
> On Tue, 2012-03-27 at 18:22 +0100, Ian Jackson wrote:
> > Stefano Stabellini writes ("RE: [Xen-devel] [PATCH 1/3] qemu-xen-traditional:
> use O_DIRECT to open disk images for IDE"):
> > > On Tue, 27 Mar 2012, Zhang, Yang Z wrote:
> > > > Doesn't cache mode have better performance than NOCACHE?
> > >
> > > Actually you are correct. I think that this patch should be dropped from
> > > the series. Of course we need O_DIRECT for QDISK otherwise we do loose
> > > correctness but considering that IDE should only be used during
> > > installation it can stay as it is.
> >
> > I don't think this assumption about IDE is correct.  To say that "IDE
> > should only be used during installation" is not an excuse for
> > providing an IDE controller which violates the usual correctness
> > rules.
> 
> The changeset which originally made this use BDRV_O_CACHE is below, do
> the arguments made there no longer apply? To my non-qemu eye it looks
> like hw/ide.c is doing an appropriate amount of bdrv_flush().
> 
> I think it is possible that we've incorrectly determined that
> BDRV_O_CACHE has issues with correctness?
> 
> My recollection is that way-back-when that installation to an emulated
> IDE device with O_DIRECT was so slow that it was deemed an acceptable
> trade-off, presumably given the understanding that IDE cache control was
> working.
> 
> I think Stefano measured it again recently, Stefano -- can you share the
> numbers you saw?
> 
> Ian.
> 
> commit 82787c6f689d869ad349df83ec3f58702afe00fe
> Author: Ian Jackson <ian.jackson@eu.citrix.com>
> Date:   Mon Mar 2 11:21:51 2009 +0000
> 
>     Override default cache mode for disk images to write-back
> 
>     Upstream qemu changed the default cache mode to write-through (ie,
>     O_DSYNC) which is much slower.  We do not need this as we have
>     explicit control of cacheing with the IDE cache control commands.
> 
>     Original patch by Yang Zhang modified by Ian Jackson.
> 
>     Signed-off-by: Yang Zhang <yang.zhang@intel.com>
>     Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> diff --git a/xenstore.c b/xenstore.c
> index 6bfcdbb..928e950 100644
> --- a/xenstore.c
> +++ b/xenstore.c
> @@ -472,7 +472,7 @@ void xenstore_parse_domain_config(int hvm_domid)
>  #ifdef CONFIG_STUBDOM
>          if (pasprintf(&danger_buf, "%s/device/vbd/%s", danger_path,
> e_danger[i]
>              continue;
> -       if (bdrv_open2(bs, danger_buf, 0 /* snapshot */, &bdrv_vbd) == 0) {
> +       if (bdrv_open2(bs, danger_buf, BDRV_O_CACHE_WB /* snapshot and
> write-bac
>             pstrcpy(bs->filename, sizeof(bs->filename), params);
>         } else
>  #endif
> @@ -498,7 +498,7 @@ void xenstore_parse_domain_config(int hvm_domid)
>                 }
>             }
>              pstrcpy(bs->filename, sizeof(bs->filename), params);
> -            if (bdrv_open2(bs, params, 0 /* snapshot */, format) < 0)
> +            if (bdrv_open2(bs, params, BDRV_O_CACHE_WB /* snapshot and
> write-ba
>                  fprintf(stderr, "qemu: could not open vbd '%s' or hard disk
> ima
>          }
> 
IIRC, start several guests at same time are very slowly w/o this patch. 

Yes, correctness is important. But in some cases, the user may put the performance at the first place. For example, our QA team has many cases which will boot many guest at same time. If using no-cache mode, they need to spend more time to wait the case finished. And this is not they wanted.
For KVM, the qemu argument allow user to determine which cache mode they like. I think we need to follow it. How about to add an option in config file to allow user to choose the cache mode and the default value can be no-cache.

best regards
yang

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE

[Stefano Stabellini]

On Thu, 29 Mar 2012, Zhang, Yang Z wrote:
> > -----Original Message-----
> > From: Ian Campbell [mailto:Ian.Campbell@citrix.com]
> > Sent: Wednesday, March 28, 2012 5:11 PM
> > To: Ian Jackson
> > Cc: Stefano Stabellini; Zhang, Yang Z; xen-devel@lists.xensource.com
> > Subject: Re: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to
> > open disk images for IDE
> > 
> > On Tue, 2012-03-27 at 18:22 +0100, Ian Jackson wrote:
> > > Stefano Stabellini writes ("RE: [Xen-devel] [PATCH 1/3] qemu-xen-traditional:
> > use O_DIRECT to open disk images for IDE"):
> > > > On Tue, 27 Mar 2012, Zhang, Yang Z wrote:
> > > > > Doesn't cache mode have better performance than NOCACHE?
> > > >
> > > > Actually you are correct. I think that this patch should be dropped from
> > > > the series. Of course we need O_DIRECT for QDISK otherwise we do loose
> > > > correctness but considering that IDE should only be used during
> > > > installation it can stay as it is.
> > >
> > > I don't think this assumption about IDE is correct.  To say that "IDE
> > > should only be used during installation" is not an excuse for
> > > providing an IDE controller which violates the usual correctness
> > > rules.
> > 
> > The changeset which originally made this use BDRV_O_CACHE is below, do
> > the arguments made there no longer apply? To my non-qemu eye it looks
> > like hw/ide.c is doing an appropriate amount of bdrv_flush().
> > 
> > I think it is possible that we've incorrectly determined that
> > BDRV_O_CACHE has issues with correctness?
> > 
> > My recollection is that way-back-when that installation to an emulated
> > IDE device with O_DIRECT was so slow that it was deemed an acceptable
> > trade-off, presumably given the understanding that IDE cache control was
> > working.
> > 
> > I think Stefano measured it again recently, Stefano -- can you share the
> > numbers you saw?
> > 
> > Ian.
> > 
> > commit 82787c6f689d869ad349df83ec3f58702afe00fe
> > Author: Ian Jackson <ian.jackson@eu.citrix.com>
> > Date:   Mon Mar 2 11:21:51 2009 +0000
> > 
> >     Override default cache mode for disk images to write-back
> > 
> >     Upstream qemu changed the default cache mode to write-through (ie,
> >     O_DSYNC) which is much slower.  We do not need this as we have
> >     explicit control of cacheing with the IDE cache control commands.
> > 
> >     Original patch by Yang Zhang modified by Ian Jackson.
> > 
> >     Signed-off-by: Yang Zhang <yang.zhang@intel.com>
> >     Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > 
> > diff --git a/xenstore.c b/xenstore.c
> > index 6bfcdbb..928e950 100644
> > --- a/xenstore.c
> > +++ b/xenstore.c
> > @@ -472,7 +472,7 @@ void xenstore_parse_domain_config(int hvm_domid)
> >  #ifdef CONFIG_STUBDOM
> >          if (pasprintf(&danger_buf, "%s/device/vbd/%s", danger_path,
> > e_danger[i]
> >              continue;
> > -       if (bdrv_open2(bs, danger_buf, 0 /* snapshot */, &bdrv_vbd) == 0) {
> > +       if (bdrv_open2(bs, danger_buf, BDRV_O_CACHE_WB /* snapshot and
> > write-bac
> >             pstrcpy(bs->filename, sizeof(bs->filename), params);
> >         } else
> >  #endif
> > @@ -498,7 +498,7 @@ void xenstore_parse_domain_config(int hvm_domid)
> >                 }
> >             }
> >              pstrcpy(bs->filename, sizeof(bs->filename), params);
> > -            if (bdrv_open2(bs, params, 0 /* snapshot */, format) < 0)
> > +            if (bdrv_open2(bs, params, BDRV_O_CACHE_WB /* snapshot and
> > write-ba
> >                  fprintf(stderr, "qemu: could not open vbd '%s' or hard disk
> > ima
> >          }
> > 
> IIRC, start several guests at same time are very slowly w/o this patch. 
> 
> Yes, correctness is important. But in some cases, the user may put the performance at the first place. For example, our QA team has many cases which will boot many guest at same time. If using no-cache mode, they need to spend more time to wait the case finished. And this is not they wanted.
> For KVM, the qemu argument allow user to determine which cache mode they like. I think we need to follow it. How about to add an option in config file to allow user to choose the cache mode and the default value can be no-cache.
 
I think this is a good argument: we could add a new cache parameter to
the disk line parser and pass it to QEMU.
However we still need to decide what is the right thing to do by
default.

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE

[Joseph Glanville]

On 29 March 2012 22:05, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 29 Mar 2012, Zhang, Yang Z wrote:
>> > -----Original Message-----
>> > From: Ian Campbell [mailto:Ian.Campbell@citrix.com]
>> > Sent: Wednesday, March 28, 2012 5:11 PM
>> > To: Ian Jackson
>> > Cc: Stefano Stabellini; Zhang, Yang Z; xen-devel@lists.xensource.com
>> > Subject: Re: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to
>> > open disk images for IDE
>> >
>> > On Tue, 2012-03-27 at 18:22 +0100, Ian Jackson wrote:
>> > > Stefano Stabellini writes ("RE: [Xen-devel] [PATCH 1/3] qemu-xen-traditional:
>> > use O_DIRECT to open disk images for IDE"):
>> > > > On Tue, 27 Mar 2012, Zhang, Yang Z wrote:
>> > > > > Doesn't cache mode have better performance than NOCACHE?
>> > > >
>> > > > Actually you are correct. I think that this patch should be dropped from
>> > > > the series. Of course we need O_DIRECT for QDISK otherwise we do loose
>> > > > correctness but considering that IDE should only be used during
>> > > > installation it can stay as it is.
>> > >
>> > > I don't think this assumption about IDE is correct.  To say that "IDE
>> > > should only be used during installation" is not an excuse for
>> > > providing an IDE controller which violates the usual correctness
>> > > rules.
>> >
>> > The changeset which originally made this use BDRV_O_CACHE is below, do
>> > the arguments made there no longer apply? To my non-qemu eye it looks
>> > like hw/ide.c is doing an appropriate amount of bdrv_flush().
>> >
>> > I think it is possible that we've incorrectly determined that
>> > BDRV_O_CACHE has issues with correctness?
>> >
>> > My recollection is that way-back-when that installation to an emulated
>> > IDE device with O_DIRECT was so slow that it was deemed an acceptable
>> > trade-off, presumably given the understanding that IDE cache control was
>> > working.
>> >
>> > I think Stefano measured it again recently, Stefano -- can you share the
>> > numbers you saw?
>> >
>> > Ian.
>> >
>> > commit 82787c6f689d869ad349df83ec3f58702afe00fe
>> > Author: Ian Jackson <ian.jackson@eu.citrix.com>
>> > Date:   Mon Mar 2 11:21:51 2009 +0000
>> >
>> >     Override default cache mode for disk images to write-back
>> >
>> >     Upstream qemu changed the default cache mode to write-through (ie,
>> >     O_DSYNC) which is much slower.  We do not need this as we have
>> >     explicit control of cacheing with the IDE cache control commands.
>> >
>> >     Original patch by Yang Zhang modified by Ian Jackson.
>> >
>> >     Signed-off-by: Yang Zhang <yang.zhang@intel.com>
>> >     Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
>> >
>> > diff --git a/xenstore.c b/xenstore.c
>> > index 6bfcdbb..928e950 100644
>> > --- a/xenstore.c
>> > +++ b/xenstore.c
>> > @@ -472,7 +472,7 @@ void xenstore_parse_domain_config(int hvm_domid)
>> >  #ifdef CONFIG_STUBDOM
>> >          if (pasprintf(&danger_buf, "%s/device/vbd/%s", danger_path,
>> > e_danger[i]
>> >              continue;
>> > -       if (bdrv_open2(bs, danger_buf, 0 /* snapshot */, &bdrv_vbd) == 0) {
>> > +       if (bdrv_open2(bs, danger_buf, BDRV_O_CACHE_WB /* snapshot and
>> > write-bac
>> >             pstrcpy(bs->filename, sizeof(bs->filename), params);
>> >         } else
>> >  #endif
>> > @@ -498,7 +498,7 @@ void xenstore_parse_domain_config(int hvm_domid)
>> >                 }
>> >             }
>> >              pstrcpy(bs->filename, sizeof(bs->filename), params);
>> > -            if (bdrv_open2(bs, params, 0 /* snapshot */, format) < 0)
>> > +            if (bdrv_open2(bs, params, BDRV_O_CACHE_WB /* snapshot and
>> > write-ba
>> >                  fprintf(stderr, "qemu: could not open vbd '%s' or hard disk
>> > ima
>> >          }
>> >
>> IIRC, start several guests at same time are very slowly w/o this patch.
>>
>> Yes, correctness is important. But in some cases, the user may put the performance at the first place. For example, our QA team has many cases which will boot many guest at same time. If using no-cache mode, they need to spend more time to wait the case finished. And this is not they wanted.
>> For KVM, the qemu argument allow user to determine which cache mode they like. I think we need to follow it. How about to add an option in config file to allow user to choose the cache mode and the default value can be no-cache.
>
> I think this is a good argument: we could add a new cache parameter to
> the disk line parser and pass it to QEMU.
> However we still need to decide what is the right thing to do by
> default.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Enabling writeback caching by default I think is probably an ill-advised choice.
The default in QEMU and KVM is writethrough as I understand it for
much the same reasons as those I noted above.

The addition of an a parameter to match the QEMU invocation
(cache=<none>,<writeback>,<writethrough>) would be most welcome.
I would suggest setting the default to writethrough as per KVM/QEMU
defaults as this is probably the least surprising choice.

Joseph.

-- 
Founder | Director | VP Research
Orion Virtualisation Solutions | www.orionvm.com.au | Phone: 1300 56
99 52 | Mobile: 0428 754 846

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE

[Stefano Stabellini]

On Thu, 29 Mar 2012, Joseph Glanville wrote:
> On 29 March 2012 22:05, Stefano Stabellini
> >> Yes, correctness is important. But in some cases, the user may put the performance at the first place. For example, our QA team has many cases which will boot many guest at same time. If using no-cache mode, they need to spend more time to wait the case finished. And this is not they wanted.
> >> For KVM, the qemu argument allow user to determine which cache mode they like. I think we need to follow it. How about to add an option in config file to allow user to choose the cache mode and the default value can be no-cache.
> >
> > I think this is a good argument: we could add a new cache parameter to
> > the disk line parser and pass it to QEMU.
> > However we still need to decide what is the right thing to do by
> > default.
> 
> Enabling writeback caching by default I think is probably an ill-advised choice.
> The default in QEMU and KVM is writethrough as I understand it for
> much the same reasons as those I noted above.
[...]
> I would suggest setting the default to writethrough as per KVM/QEMU
> defaults as this is probably the least surprising choice.

I don't think it would be wise to switch from WB to WT by default,
considering the incredible difference in performance between the two.
Also as from my other email in this thread, I don't think it is
actually necessary to use WT to enforce correctness in all cases but
QCOW2. Moreover BZ 572825 has been fixed in upstream QEMU so it only
affects qemu-xen-traditional now.

As a consequence I think the best default behaviour we can have for
qemu-xen-traditional is:


                    IDE         QDISK
---------------------------------------
RAW            |    WB    |     NOCACHE
QCOW/QCOW2     |    WT    |     NOCACHE



For qemu-xen:

                    IDE         QDISK
---------------------------------------
RAW            |    WB    |     NOCACHE
QCOW/QCOW2     |    WB    |     NOCACHE


We should document that the IDE interface for HVM guests doesn't have the
same level of safeguards than QDISK in case of sudden host power downs
(as it has always been since that patch in 2009).



> The addition of an a parameter to match the QEMU invocation
> (cache=<none>,<writeback>,<writethrough>) would be most welcome.

Agreed.

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE

[Zhang, Yang Z]

> -----Original Message-----
> From: Joseph Glanville [mailto:joseph.glanville@orionvm.com.au]
> Sent: Thursday, March 29, 2012 7:51 PM
> To: Stefano Stabellini
> Cc: Zhang, Yang Z; xen-devel@lists.xensource.com; Ian Jackson; Ian Campbell
> Subject: Re: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to
> open disk images for IDE
> 
> On 29 March 2012 22:05, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > On Thu, 29 Mar 2012, Zhang, Yang Z wrote:
> >> > -----Original Message-----
> >> > From: Ian Campbell [mailto:Ian.Campbell@citrix.com]
> >> > Sent: Wednesday, March 28, 2012 5:11 PM
> >> > To: Ian Jackson
> >> > Cc: Stefano Stabellini; Zhang, Yang Z; xen-devel@lists.xensource.com
> >> > Subject: Re: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT
> to
> >> > open disk images for IDE
> >> >
> >> > On Tue, 2012-03-27 at 18:22 +0100, Ian Jackson wrote:
> >> > > Stefano Stabellini writes ("RE: [Xen-devel] [PATCH 1/3]
> qemu-xen-traditional:
> >> > use O_DIRECT to open disk images for IDE"):
> >> > > > On Tue, 27 Mar 2012, Zhang, Yang Z wrote:
> >> > > > > Doesn't cache mode have better performance than NOCACHE?
> >> > > >
> >> > > > Actually you are correct. I think that this patch should be dropped from
> >> > > > the series. Of course we need O_DIRECT for QDISK otherwise we do
> loose
> >> > > > correctness but considering that IDE should only be used during
> >> > > > installation it can stay as it is.
> >> > >
> >> > > I don't think this assumption about IDE is correct.  To say that "IDE
> >> > > should only be used during installation" is not an excuse for
> >> > > providing an IDE controller which violates the usual correctness
> >> > > rules.
> >> >
> >> > The changeset which originally made this use BDRV_O_CACHE is below, do
> >> > the arguments made there no longer apply? To my non-qemu eye it looks
> >> > like hw/ide.c is doing an appropriate amount of bdrv_flush().
> >> >
> >> > I think it is possible that we've incorrectly determined that
> >> > BDRV_O_CACHE has issues with correctness?
> >> >
> >> > My recollection is that way-back-when that installation to an emulated
> >> > IDE device with O_DIRECT was so slow that it was deemed an acceptable
> >> > trade-off, presumably given the understanding that IDE cache control was
> >> > working.
> >> >
> >> > I think Stefano measured it again recently, Stefano -- can you share the
> >> > numbers you saw?
> >> >
> >> > Ian.
> >> >
> >> > commit 82787c6f689d869ad349df83ec3f58702afe00fe
> >> > Author: Ian Jackson <ian.jackson@eu.citrix.com>
> >> > Date:   Mon Mar 2 11:21:51 2009 +0000
> >> >
> >> >     Override default cache mode for disk images to write-back
> >> >
> >> >     Upstream qemu changed the default cache mode to write-through
> (ie,
> >> >     O_DSYNC) which is much slower.  We do not need this as we have
> >> >     explicit control of cacheing with the IDE cache control commands.
> >> >
> >> >     Original patch by Yang Zhang modified by Ian Jackson.
> >> >
> >> >     Signed-off-by: Yang Zhang <yang.zhang@intel.com>
> >> >     Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> >> >
> >> > diff --git a/xenstore.c b/xenstore.c
> >> > index 6bfcdbb..928e950 100644
> >> > --- a/xenstore.c
> >> > +++ b/xenstore.c
> >> > @@ -472,7 +472,7 @@ void xenstore_parse_domain_config(int
> hvm_domid)
> >> >  #ifdef CONFIG_STUBDOM
> >> >          if (pasprintf(&danger_buf, "%s/device/vbd/%s", danger_path,
> >> > e_danger[i]
> >> >              continue;
> >> > -       if (bdrv_open2(bs, danger_buf, 0 /* snapshot */, &bdrv_vbd) == 0)
> {
> >> > +       if (bdrv_open2(bs, danger_buf, BDRV_O_CACHE_WB /* snapshot
> and
> >> > write-bac
> >> >             pstrcpy(bs->filename, sizeof(bs->filename), params);
> >> >         } else
> >> >  #endif
> >> > @@ -498,7 +498,7 @@ void xenstore_parse_domain_config(int
> hvm_domid)
> >> >                 }
> >> >             }
> >> >              pstrcpy(bs->filename, sizeof(bs->filename), params);
> >> > -            if (bdrv_open2(bs, params, 0 /* snapshot */, format) < 0)
> >> > +            if (bdrv_open2(bs, params, BDRV_O_CACHE_WB /*
> snapshot and
> >> > write-ba
> >> >                  fprintf(stderr, "qemu: could not open vbd '%s' or
> hard disk
> >> > ima
> >> >          }
> >> >
> >> IIRC, start several guests at same time are very slowly w/o this patch.
> >>
> >> Yes, correctness is important. But in some cases, the user may put the
> performance at the first place. For example, our QA team has many cases which
> will boot many guest at same time. If using no-cache mode, they need to spend
> more time to wait the case finished. And this is not they wanted.
> >> For KVM, the qemu argument allow user to determine which cache mode
> they like. I think we need to follow it. How about to add an option in config file to
> allow user to choose the cache mode and the default value can be no-cache.
> >
> > I think this is a good argument: we could add a new cache parameter to
> > the disk line parser and pass it to QEMU.
> > However we still need to decide what is the right thing to do by
> > default.
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 
> Enabling writeback caching by default I think is probably an ill-advised choice.
> The default in QEMU and KVM is writethrough as I understand it for
> much the same reasons as those I noted above.
> 
> The addition of an a parameter to match the QEMU invocation
> (cache=<none>,<writeback>,<writethrough>) would be most welcome.
> I would suggest setting the default to writethrough as per KVM/QEMU
> defaults as this is probably the least surprising choice.
> 
Agree.

best regards
yang

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE"):
> commit 82787c6f689d869ad349df83ec3f58702afe00fe
> Author: Ian Jackson <ian.jackson@eu.citrix.com>
> Date:   Mon Mar 2 11:21:51 2009 +0000
> 
>     Override default cache mode for disk images to write-back
>     
>     Upstream qemu changed the default cache mode to write-through (ie,
>     O_DSYNC) which is much slower.  We do not need this as we have
>     explicit control of cacheing with the IDE cache control commands.
...

OK, so the reason for this is as follows:

There are IDE commands for controlling cacheing in the disk.  These
are the commands 0x02 and 0x82 (near l.2666 of hw/ide.c in
qemu-xen-unstable).  If the host has enabled the write cache, the disk
is allowed to buffer written data in its own ram.  The host is
expected to either disable the cache, or flush the cache
(WIN_FLUSH_CACHE, near l.2717) as and when appropriate.

We implement this in qemu-xen-unstable as follows:

The bdrv_write calls are expected to be write-back cached.  The cache
enable/disable commands set a variable write_cache in the ide code.
If write_cache is 0, we run bdrv_flush after each write.  In any case
we run bdrv_flush when we get a cache flush command.  bdrv_flush
calls, effectively, fsync().

So I think qemu-xen-unstable the use of write-back cacheing for
bdrv_write is correct.  (Apart from the wrinkle that the emulated
drive cache, write_cache, is enabled by default - but I wouldn't be
surprised if that accurately emulated the behaviour of real disks.)

How is this done in qemu-upstream ?

Ian.

Re: [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE

[Stefano Stabellini]

On Thu, 29 Mar 2012, Ian Jackson wrote:

> Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/3] qemu-xen-traditional: use O_DIRECT to open disk images for IDE"):
> > commit 82787c6f689d869ad349df83ec3f58702afe00fe
> > Author: Ian Jackson <ian.jackson@eu.citrix.com>
> > Date:   Mon Mar 2 11:21:51 2009 +0000
> > 
> >     Override default cache mode for disk images to write-back
> >     
> >     Upstream qemu changed the default cache mode to write-through (ie,
> >     O_DSYNC) which is much slower.  We do not need this as we have
> >     explicit control of cacheing with the IDE cache control commands.
> ...
> 
> OK, so the reason for this is as follows:
> 
> There are IDE commands for controlling cacheing in the disk.  These
> are the commands 0x02 and 0x82 (near l.2666 of hw/ide.c in
> qemu-xen-unstable).  If the host has enabled the write cache, the disk
> is allowed to buffer written data in its own ram.  The host is
> expected to either disable the cache, or flush the cache
> (WIN_FLUSH_CACHE, near l.2717) as and when appropriate.
> 
> We implement this in qemu-xen-unstable as follows:
> 
> The bdrv_write calls are expected to be write-back cached.  The cache
> enable/disable commands set a variable write_cache in the ide code.
> If write_cache is 0, we run bdrv_flush after each write.  In any case
> we run bdrv_flush when we get a cache flush command.  bdrv_flush
> calls, effectively, fsync().
> 
> So I think qemu-xen-unstable the use of write-back cacheing for
> bdrv_write is correct.  (Apart from the wrinkle that the emulated
> drive cache, write_cache, is enabled by default - but I wouldn't be
> surprised if that accurately emulated the behaviour of real disks.)

You have missed my other emails on this thread about other disk formats
and related bug reports.



> How is this done in qemu-upstream ?
 
WIN_FLUSH_CACHE calls ide_flush_cache that calls bdrv_aio_flush.

[PATCH 2/3] qemu-xen-traditional: use O_DIRECT to open disk images with QDISK

[Stefano Stabellini]

Also enable batch_maps, use_aio and disable syncwrite.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen_disk.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 6aebb77..b1d6985 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -46,11 +46,11 @@
 
 /* ------------------------------------------------------------- */
 
-static int syncwrite    = 1;
-static int batch_maps   = 0;
+static int syncwrite    = 0;
+static int batch_maps   = 1;
 
 static int max_requests = 32;
-static int use_aio      = 0;
+static int use_aio      = 1;
 
 /* ------------------------------------------------------------- */
 
@@ -617,12 +617,13 @@ static int blk_init(struct XenDevice *xendev)
 	return -1;
 
     /* read-only ? */
+    qflags = BDRV_O_NOCACHE;
     if (strcmp(blkdev->mode, "w") == 0) {
 	mode   = O_RDWR;
-	qflags = BDRV_O_RDWR;
+	qflags |= BDRV_O_RDWR;
     } else {
 	mode   = O_RDONLY;
-	qflags = BDRV_O_RDONLY;
+	qflags |= BDRV_O_RDONLY;
 	info  |= VDISK_READONLY;
     }
 
-- 
1.7.2.5

[PATCH 3/3] qemu-xen-traditional: QDISK fixes

[Stefano Stabellini]

- if ioreq->postsync call bdrv_flush when the operation is actually
completed;

- do not increment aio_inflight when not submitting any operations.


Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen_disk.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index b1d6985..5db58ac 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -382,6 +382,8 @@ static void qemu_aio_complete(void *opaque, int ret)
     ioreq->aio_inflight--;
     if (ioreq->aio_inflight > 0)
         return;
+    if (ioreq->postsync)
+	bdrv_flush(ioreq->blkdev->bs);
 
     ioreq->status = ioreq->aio_errors ? BLKIF_RSP_ERROR : BLKIF_RSP_OKAY;
     ioreq_unmap(ioreq);
@@ -409,9 +411,9 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
 	break;
     case BLKIF_OP_WRITE:
     case BLKIF_OP_WRITE_BARRIER:
-        ioreq->aio_inflight++;
         if (!ioreq->req.nr_segments)
             break;
+        ioreq->aio_inflight++;
         bdrv_aio_writev(blkdev->bs, ioreq->start / BLOCK_SIZE,
                         &ioreq->v, ioreq->v.size / BLOCK_SIZE,
                         qemu_aio_complete, ioreq);
@@ -421,8 +423,6 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
 	goto err;
     }
 
-    if (ioreq->postsync)
-	bdrv_flush(blkdev->bs); /* FIXME: aio_flush() ??? */
     qemu_aio_complete(ioreq, 0);
 
     return 0;
-- 
1.7.2.5

Re: [PATCH 0/3] qemu-xen-traditional: disk performance improvements

[Ian Jackson]

Stefano Stabellini writes ("[PATCH 0/3] qemu-xen-traditional: disk performance improvements"):
> this small patch series enables the O_DIRECT flag to open disk images
> for both the IDE and xen_disk interface.
> Also it includes few fixes for xen_disk.
> 
> Stefano Stabellini (3):
>       qemu-xen-traditional: use O_DIRECT to open disk images for IDE
>       qemu-xen-traditional: use O_DIRECT to open disk images with QDISK
>       qemu-xen-traditional: QDISK fixes

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH 0/3] qemu-xen-traditional: disk performance improvements

[Ian Jackson]

Stefano Stabellini writes ("[Xen-devel] [PATCH 0/3] qemu-xen-traditional: disk performance improvements"):
> Hi all,
> this small patch series enables the O_DIRECT flag to open disk images
> for both the IDE and xen_disk interface.
> Also it includes few fixes for xen_disk.
> 
> Stefano Stabellini (3):
>       qemu-xen-traditional: use O_DIRECT to open disk images for IDE
>       qemu-xen-traditional: use O_DIRECT to open disk images with QDISK
>       qemu-xen-traditional: QDISK fixes

Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH 0/3] qemu-xen-traditional: disk performance improvements

[Stefano Stabellini]

On Mon, 2 Apr 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] [PATCH 0/3] qemu-xen-traditional: disk performance improvements"):
> > Hi all,
> > this small patch series enables the O_DIRECT flag to open disk images
> > for both the IDE and xen_disk interface.
> > Also it includes few fixes for xen_disk.
> > 
> > Stefano Stabellini (3):
> >       qemu-xen-traditional: use O_DIRECT to open disk images for IDE
> >       qemu-xen-traditional: use O_DIRECT to open disk images with QDISK
> >       qemu-xen-traditional: QDISK fixes
> 
> Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 

Could you please backport these patches to qemu-xen-4.1-testing?

Re: [PATCH 0/3] qemu-xen-traditional: disk performance improvements

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 0/3] qemu-xen-traditional: disk performance improvements"):
> On Mon, 2 Apr 2012, Ian Jackson wrote:
> > Stefano Stabellini writes ("[Xen-devel] [PATCH 0/3] qemu-xen-traditional: disk performance improvements"):
> > > Hi all,
> > > this small patch series enables the O_DIRECT flag to open disk images
> > > for both the IDE and xen_disk interface.
> > > Also it includes few fixes for xen_disk.
> > > 
> > > Stefano Stabellini (3):
> > >       qemu-xen-traditional: use O_DIRECT to open disk images for IDE
> > >       qemu-xen-traditional: use O_DIRECT to open disk images with QDISK
> > >       qemu-xen-traditional: QDISK fixes
> > 
> > Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > 
> 
> Could you please backport these patches to qemu-xen-4.1-testing?

Done.

Thanks,
Ian.