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?
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).
Note that there is actually a case where qcow2 should support
BDRV_REQ_MAY_UNMAP, which is for v2 images. Currently, we just return
-ENOTSUP for them, but if we don't have a backing file and
BDRV_REQ_MAY_UNMAP is set, we could go on and make qcow2_zero_clusters()
work for them.
Max
> } else {
> @@ -1595,7 +1595,8 @@ static int zero_single_l2(BlockDriverState *bs,
> uint64_t offset,
> return nb_clusters;
> }
>
> -int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int
> nb_sectors)
> +int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int
> nb_sectors,
> + int flags)
> {
> BDRVQcow2State *s = bs->opaque;
> uint64_t nb_clusters;
> @@ -1612,7 +1613,7 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t
> offset, int nb_sectors)
> s->cache_discards = true;
>
> while (nb_clusters > 0) {
> - ret = zero_single_l2(bs, offset, nb_clusters);
> + ret = zero_single_l2(bs, offset, nb_clusters, flags);
> if (ret < 0) {
> goto fail;
> }
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 0e53a4d..474f244 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1154,6 +1154,7 @@ static int qcow2_open(BlockDriverState *bs, QDict
> *options, int flags,
>
> /* Initialise locks */
> qemu_co_mutex_init(&s->lock);
> + bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP;
>
> /* Repair image if dirty */
> if (!(flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) && !bs->read_only &&
> @@ -2476,7 +2477,7 @@ static coroutine_fn int
> qcow2_co_pwrite_zeroes(BlockDriverState *bs,
> trace_qcow2_pwrite_zeroes(qemu_coroutine_self(), offset, count);
>
> /* Whatever is left can use real zero clusters */
> - ret = qcow2_zero_clusters(bs, offset, count >> BDRV_SECTOR_BITS);
> + ret = qcow2_zero_clusters(bs, offset, count >> BDRV_SECTOR_BITS, flags);
> qemu_co_mutex_unlock(&s->lock);
>
> return ret;
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 9ce5a37..92203a8 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -547,7 +547,8 @@ uint64_t
> qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
> int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m);
> int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
> int nb_sectors, enum qcow2_discard_type type, bool full_discard);
> -int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int
> nb_sectors);
> +int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int
> nb_sectors,
> + int flags);
>
> int qcow2_expand_zero_clusters(BlockDriverState *bs,
> BlockDriverAmendStatusCB *status_cb,
>
signature.asc
Description: OpenPGP digital signature
