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 <f...@redhat.com> > > --- > > 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