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

[Kevin Wolf]

Am 29.09.2016 um 11:55 hat Fam Zheng geschrieben:
> 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 <address@hidden>
> > > ---
> > >  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);
>         }

What I mean is that in the second case, you're still uselessly
deallocating the cluster on the qcow2 level while you can't reclaim it
on the filesystem level. So it would be better to leave it allocated in
qcow2, too, so that you don't get an expensive reallocation the next
time you write to it.

Kevin