qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
Date: Wed, 28 Sep 2016 18:11:31 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0

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 <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);

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,
> 


Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]