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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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