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

[Joseph Glanville <joseph.glanville@xxxxxxxxxxxxxx>]

On 29 March 2012 22:05, Stefano Stabellini
<stefano.stabellini@xxxxxxxxxxxxx> wrote:
> On Thu, 29 Mar 2012, Zhang, Yang Z wrote:
>> > -----Original Message-----
>> > From: Ian Campbell [mailto:Ian.Campbell@xxxxxxxxxx]
>> > Sent: Wednesday, March 28, 2012 5:11 PM
>> > To: Ian Jackson
>> > Cc: Stefano Stabellini; Zhang, Yang Z; xen-devel@xxxxxxxxxxxxxxxxxxx
>> > 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@xxxxxxxxxxxxx>
>> > 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@xxxxxxxxx>
>> >     Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>> >
>> > 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@xxxxxxxxxxxxx
> 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

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