Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP

[Fam Zheng <famz@redhat.com>]

On Thu, 09/29 11:29, Kevin Wolf wrote:
> Am 28.09.2016 um 09:04 hat Fam Zheng geschrieben:
> > Handling this is similar to what is done to the L2 entry in the case of
> > compressed clusters.
> > 
> > Signed-off-by: Fam Zheng <famz@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);
> >          } else {
> 
> I don't think we should always do this for BDRV_REQ_MAY_UNMAP. The user
> may want to keep the image fully allocated even if the guest (or block
> job etc.) thinks this can be discarded.
> 
> When we discussed this in another email thread (?), I think I suggested
> checking whether the image was opened with pass-discard-request=on. If
> so, go ahead and deallocate the cluster, otherwise keep it allocated.
> 
> In those cases where pass-discard-request=off, it doesn't make a lot of
> sense to deallocate the cluster anyway because it won't shrink the file
> size.

I think this patch does what you mean:

    $ strace -f -e fallocate qemu-io \
        -c 'open -o pass-discard-request=on /var/tmp/test' \
        -c 'write 0 1M' \
        -c 'write -u -z 0 1M' \
        2>&1 >/dev/null | \
        grep fallocate
    [pid 17548] fallocate(17, FALLOC_FL_KEEP_SIZE|FALLOC_FL_PUNCH_HOLE, 327680, 1048576) = 0
    $ strace -f -e fallocate qemu-io \
        -c 'open -o pass-discard-request=off /var/tmp/test' \
        -c 'write 0 1M' \
        -c 'write -u -z 0 1M' \
        2>&1 >/dev/null | \
        grep fallocate

Because there is another check of pass-discard-request value in
update_refcount:

        if (refcount == 0 && s->discard_passthrough[type]) {
            update_refcount_discard(bs, cluster_offset, s->cluster_size);
        }

Fam