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

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

> -----Original Message-----
> From: Joseph Glanville [mailto:joseph.glanville@xxxxxxxxxxxxxx]
> Sent: Thursday, March 29, 2012 7:51 PM
> To: Stefano Stabellini
> Cc: Zhang, Yang Z; xen-devel@xxxxxxxxxxxxxxxxxxx; 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@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.
> 
Agree.

best regards
yang

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