From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53871) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UnUny-0003LW-8Q for qemu-devel@nongnu.org; Fri, 14 Jun 2013 10:16:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UnUnw-00027z-Fm for qemu-devel@nongnu.org; Fri, 14 Jun 2013 10:16:42 -0400 Received: from mail-qc0-x231.google.com ([2607:f8b0:400d:c01::231]:56130) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UnUnw-00027m-CU for qemu-devel@nongnu.org; Fri, 14 Jun 2013 10:16:40 -0400 Received: by mail-qc0-f177.google.com with SMTP id n1so314929qcx.22 for ; Fri, 14 Jun 2013 07:16:40 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <51BB25C3.4000904@redhat.com> Date: Fri, 14 Jun 2013 10:16:35 -0400 From: Paolo Bonzini MIME-Version: 1.0 References: <1371124063-12971-1-git-send-email-kwolf@redhat.com> <1371124063-12971-4-git-send-email-kwolf@redhat.com> <51BA4344.7070608@redhat.com> <20130614083110.GB2389@dhcp-200-207.str.redhat.com> In-Reply-To: <20130614083110.GB2389@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/5] qcow2: Options to enable discard for freed clusters List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, stefanha@redhat.com Il 14/06/2013 04:31, Kevin Wolf ha scritto: >>> > > + s->discard_passthrough[QCOW2_DISCARD_NEVER] = false, >>> > > + s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true, >>> > > + s->discard_passthrough[QCOW2_DISCARD_REQUEST] = >>> > > + qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST, >>> > > + flags & BDRV_O_UNMAP), >> > >> > I think there should not be two ways to enable it, it is confusing. > Hm, yes... But it's also confusing to have qcow2 provide an incomplete > set of categories. Maybe we shouldn't have introduced -drive discard=... > as a global option to begin with. > >>> > > + s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] = >>> > > + qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true), >>> > > + s->discard_passthrough[QCOW2_DISCARD_OTHER] = >>> > > + qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false), >> > >> > Please document the defaults in qcow2_runtime_opts. (BTW, what is the >> > rationale?) > The idea was that discard is slow and therefore disabled by default, > except when you're doing an expensive snapshot operation that can > potentially free a lot of space at once with not too many requests, so > there it's enabled. And if you said -drive discard=on, you obviously > want guest requests to take effect. > > We could let QCOW2_OPT_DISCARD_OTHER default to BDRV_O_UNMAP as well if > you prefer. It looks like QCOW2_OPT_DISCARD_OTHER is a rare case, so I don't mind leaving it as default to false. It won't waste more than a few clusters. In the end discard_snapshot and discard_other should rarely be needed in practice, so I don't think having discard=... is a mistake. Too many knobs won't really be needed. In fact, perhaps we do not need discard_snapshot and discard_request, only discard_other. discard_snapshot can be replaced by file.discard=ignore, discard_request by discard=unmap. Paolo