From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49227) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bpQyy-0005if-Lz for qemu-devel@nongnu.org; Wed, 28 Sep 2016 22:21:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bpQyx-0000eC-Di for qemu-devel@nongnu.org; Wed, 28 Sep 2016 22:21:56 -0400 Date: Thu, 29 Sep 2016 10:21:47 +0800 From: Fam Zheng Message-ID: <20160929022147.GB6412@lemon> References: <1475046261-15679-1-git-send-email-famz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-devel@nongnu.org, Kevin Wolf , qemu-block@nongnu.org On Wed, 09/28 18:11, Max Reitz wrote: > On 28.09.2016 09:04, Fam Zheng wrote: > > Handling this is similar to what is done to the L2 entry in the case of > > compressed clusters. > > > > Signed-off-by: Fam Zheng > > --- > > block/qcow2-cluster.c | 9 +++++---- > > block/qcow2.c | 3 ++- > > block/qcow2.h | 3 ++- > > 3 files changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > > index 61d1ffd..928c1e2 100644 > > --- a/block/qcow2-cluster.c > > +++ b/block/qcow2-cluster.c > > @@ -1558,7 +1558,7 @@ fail: > > * clusters. > > */ > > static int zero_single_l2(BlockDriverState *bs, uint64_t offset, > > - uint64_t nb_clusters) > > + uint64_t nb_clusters, int flags) > > { > > BDRVQcow2State *s = bs->opaque; > > uint64_t *l2_table; > > @@ -1582,7 +1582,7 @@ static int zero_single_l2(BlockDriverState *bs, uint64_t offset, > > > > /* Update L2 entries */ > > qcow2_cache_entry_mark_dirty(bs, s->l2_table_cache, l2_table); > > - if (old_offset & QCOW_OFLAG_COMPRESSED) { > > + if (old_offset & QCOW_OFLAG_COMPRESSED || flags & BDRV_REQ_MAY_UNMAP) { > > l2_table[l2_index + i] = cpu_to_be64(QCOW_OFLAG_ZERO); > > qcow2_free_any_clusters(bs, old_offset, 1, QCOW2_DISCARD_REQUEST); > > I don't quite understand the reasoning behind this. How is this more > efficient than just using the existing path where we don't discard anything? It's more efficient in disk space. I didn't mention because it is so not specific to this, but: what virt-sparsify does is creating an overlay -> fstrim it -> qemu-img commit. This flow revealed to me that BDRV_REQ_MAY_UNMAP should have been honored this way (after a hint of "how" by Kevin). > > Note that BDRV_REQ_MAY_UNMAP does not mean "Yes, please discard" but > just "You may discard if it's easier for you". But it's actually not > easier for us, so I don't see why we're doing it. > > As far as I can guess you actually want some way to tell a block driver > to actually make an effort to discard clusters as long they then read > back as zero (which is why you cannot simply use bdrv_pdiscard()). > However, I think this would require a new flag called > BDRV_REQ_SHOULD_UNMAP (which should imply BDRV_REQ_MAY_UNMAP). This flag doesn't make sense to me, if the protocol doesn't know how to unmap, it can ignore BDRV_REQ_MAY_UNMAP, but not BDRV_REQ_SHOULD_UNMAP. It just complicates things a little. Fam