Am 30.09.2016 um 04:04 hat Fam Zheng geschrieben:
> On Thu, 09/29 12:39, Kevin Wolf wrote:
> > 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 <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);
> > > > > } 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.
> >
>
> Like this?
That's what I had in mind, but actually I think Paolo is right and the
BDRV_O_UNMAP check in bdrv_co_pwrite_zeroes() already does all that is
needed. So the approach of the original patch should be okay.
Kevin
> ---
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 928c1e2..3d3cea4 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1582,7 +1582,9 @@ 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 || flags &
> BDRV_REQ_MAY_UNMAP) {
> + if (old_offset & QCOW_OFLAG_COMPRESSED ||
> + ((flags & BDRV_REQ_MAY_UNMAP) &&
> + s->discard_passthrough[QCOW2_DISCARD_REQUEST])) {
> l2_table[l2_index + i] = cpu_to_be64(QCOW_OFLAG_ZERO);
> qcow2_free_any_clusters(bs, old_offset, 1,
> QCOW2_DISCARD_REQUEST);
> } else {
