From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Wolf Subject: Re: [PATCH] qemu-img: Add nocache command line option Date: Wed, 15 Jun 2011 13:45:02 +0200 Message-ID: <4DF89B3E.9010902@redhat.com> References: <295713412.684520.1308136637628.JavaMail.root@zmail02.collab.prod.int.phx2.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Avi Kivity To: Federico Simoncelli Return-path: Received: from mx1.redhat.com ([209.132.183.28]:63128 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754819Ab1FOLmJ (ORCPT ); Wed, 15 Jun 2011 07:42:09 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p5FBg9YN016465 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 15 Jun 2011 07:42:09 -0400 In-Reply-To: <295713412.684520.1308136637628.JavaMail.root@zmail02.collab.prod.int.phx2.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Am 15.06.2011 13:17, schrieb Federico Simoncelli: > ----- Original Message ----- >> From: "Kevin Wolf" >> To: "Avi Kivity" >> Cc: "Federico Simoncelli" , kvm@vger.kernel.org >> Sent: Wednesday, June 15, 2011 12:50:21 PM >> Subject: Re: [PATCH] qemu-img: Add nocache command line option >> Am 14.06.2011 15:13, schrieb Avi Kivity: >>> On 06/14/2011 03:09 PM, Federico Simoncelli wrote: >>>> qemu-img currently writes images using writeback and filling up >>>> the cache buffers which are then flushed by the kernel preventing >>>> other processes from accessing the storage. >>>> This is particularly bad in cluster environments where time-based >>>> algorithms might be in place and accessing the storage within >>>> certain timeouts is critical. >>>> This patch adds the option to use nocache when other solutions >>>> (eg: cgroups) are not available. >> >> Your patch confuses a write-through cache (cache=writethrough == >> O_SYNC) >> with bypassing the cache (cache=none == O_DIRECT), which are entirely >> different. > > I know the difference, how am I confusing them in the patch? > I am using BDRV_O_NOCACHE which is translated to O_DIRECT, which is > what I wanted to use (as per commit message: nocache). > Maybe I overlooked something, can you point me to the code parts you > suppose are wrong? The first thing that I noticed is that you call it write-through in the help message. But you also unset BDRV_O_CACHE_WB, while cache=none is a write-back cache mode, in fact. >>> Please copy qemu-devel@nongnu.org as this is not a kvm-specific >>> patch. >>> >>>> diff --git a/qemu-img.c b/qemu-img.c >>>> index 4f162d1..c10e4a6 100644 >>>> --- a/qemu-img.c >>>> +++ b/qemu-img.c >>>> @@ -78,6 +78,7 @@ static void help(void) >>>> " rebasing in this case (useful for renaming the >>>> backing file)\n" >>>> " '-h' with or without a command shows this help and >>>> lists the supported formats\n" >>>> " '-p' show progress of command (only certain >>>> commands)\n" >>>> + " '-t' use write-through (no cache), valid with: convert, commit >>>> and rebase\n" >>>> "\n" >>>> "Parameters to snapshot subcommand:\n" >>>> " 'snapshot' is the name of the snapshot to create, >>>> apply or delete\n" >>> >>> IMO better to use the existing qemu option format, say -o >>> cache=writeback|writethrough|none|volatile. >> >> No, -o is used with qemu-img create for options that change what the >> image will look like (e.g. setting flags in the image header). This >> options is about the cache mode that is used for various operations. > > Actually I am just working over this. I am going to add a qemu-img > specific option, roughly something like this: > > static QEMUOptionParameter qemuimg_options[] = { > { > .name = QEMUIMG_OPT_CACHE, > .type = OPT_STRING, > .help = "Write cache method" > }, > { NULL } > }; > [...] > create_options = append_option_parameters(create_options, > qemuimg_options); > [...] > /* Setting output cache flag */ > out_flags = BDRV_O_RDWR | BDRV_O_NO_FLUSH; > ret = qemuimg_option_cache(param, &out_flags); > if (ret < 0) { > goto out; > } My concerns were not about the implementation (I'm sure that you can do that), but rather on a conceptual level. Persistent image options are semantically different from options only used for the qemu-img operation, so mixing the two things gives you a bad user interface. Kevin