[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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. 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |