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

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

[Fam Zheng]

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 {
@@ -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,
-- 
2.7.4

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4803 bytes --]

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

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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

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

[Fam Zheng]

On Wed, 09/28 18:11, Max Reitz wrote:
> 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 <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);
> 
> 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?

It's more efficient in disk space. I didn't mention because it is so not
specific to this, but: what virt-sparsify does is creating an overlay -> fstrim
it -> qemu-img commit. This flow revealed to me that BDRV_REQ_MAY_UNMAP should
have been honored this way (after a hint of "how" by Kevin).

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

This flag doesn't make sense to me, if the protocol doesn't know how to unmap,
it can ignore BDRV_REQ_MAY_UNMAP, but not BDRV_REQ_SHOULD_UNMAP. It just
complicates things a little.

Fam

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

[Paolo Bonzini]

On 29/09/2016 04:21, Fam Zheng wrote:
> On Wed, 09/28 18:11, Max Reitz wrote:
>> 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).
> 
> This flag doesn't make sense to me, if the protocol doesn't know how to unmap,
> it can ignore BDRV_REQ_MAY_UNMAP, but not BDRV_REQ_SHOULD_UNMAP. It just
> complicates things a little.

I don't think we actually have a use for a "MAY" unmap flag.  Either we
keep the not-so-perfect name or we replace MAY_UNMAP with "should" or
"want" or "would_like" unmap...  But Fam's patch does do what was
intended for the flag (which is the equivalent of the UNMAP bit in the
SCSI WRITE SAME command).

Paolo

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

[Fam Zheng]

On Thu, 09/29 09:58, Paolo Bonzini wrote:
> 
> 
> On 29/09/2016 04:21, Fam Zheng wrote:
> > On Wed, 09/28 18:11, Max Reitz wrote:
> >> 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).
> > 
> > This flag doesn't make sense to me, if the protocol doesn't know how to unmap,
> > it can ignore BDRV_REQ_MAY_UNMAP, but not BDRV_REQ_SHOULD_UNMAP. It just
> > complicates things a little.
> 
> I don't think we actually have a use for a "MAY" unmap flag.  Either we
> keep the not-so-perfect name or we replace MAY_UNMAP with "should" or
> "want" or "would_like" unmap...  But Fam's patch does do what was
> intended for the flag (which is the equivalent of the UNMAP bit in the
> SCSI WRITE SAME command).

After reading rfc2119, now I agree that "SHOULD" is better. :)

Fam

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1382 bytes --]

On 29.09.2016 10:10, Fam Zheng wrote:
> On Thu, 09/29 09:58, Paolo Bonzini wrote:
>>
>>
>> On 29/09/2016 04:21, Fam Zheng wrote:
>>> On Wed, 09/28 18:11, Max Reitz wrote:
>>>> 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).
>>>
>>> This flag doesn't make sense to me, if the protocol doesn't know how to unmap,
>>> it can ignore BDRV_REQ_MAY_UNMAP, but not BDRV_REQ_SHOULD_UNMAP. It just
>>> complicates things a little.
>>
>> I don't think we actually have a use for a "MAY" unmap flag.  Either we
>> keep the not-so-perfect name or we replace MAY_UNMAP with "should" or
>> "want" or "would_like" unmap...  But Fam's patch does do what was
>> intended for the flag (which is the equivalent of the UNMAP bit in the
>> SCSI WRITE SAME command).
> 
> After reading rfc2119, now I agree that "SHOULD" is better. :)

That's OK with me, then.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

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

[Kevin Wolf]

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.

Kevin

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

[Fam Zheng]

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

Re: [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 <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);
>         }

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

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

[Paolo Bonzini]

On 29/09/2016 12:39, Kevin Wolf wrote:
>> > 
>> > 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.

But if you do a qemu-img convert, the deallocated cluster wouldn't be in
the destination.

Paolo

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

[Kevin Wolf]

Am 29.09.2016 um 14:14 hat Paolo Bonzini geschrieben:
> On 29/09/2016 12:39, Kevin Wolf wrote:
> >> > 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.
> 
> But if you do a qemu-img convert, the deallocated cluster wouldn't be in
> the destination.

Right. I still think that there has to be an option to keep the image
fully allocated. Perhaps what we really need to check is BDRV_O_UNMAP.

Kevin

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

[Paolo Bonzini]

On 29/09/2016 14:48, Kevin Wolf wrote:
> Am 29.09.2016 um 14:14 hat Paolo Bonzini geschrieben:
>> On 29/09/2016 12:39, Kevin Wolf wrote:
>>>>> 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.
>>
>> But if you do a qemu-img convert, the deallocated cluster wouldn't be in
>> the destination.
> 
> Right. I still think that there has to be an option to keep the image
> fully allocated. Perhaps what we really need to check is BDRV_O_UNMAP.

Duh, of course it is.

Paolo

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

[Paolo Bonzini]

On 29/09/2016 14:50, Paolo Bonzini wrote:
> 
> 
> On 29/09/2016 14:48, Kevin Wolf wrote:
>> Am 29.09.2016 um 14:14 hat Paolo Bonzini geschrieben:
>>> On 29/09/2016 12:39, Kevin Wolf wrote:
>>>>>> 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.
>>>
>>> But if you do a qemu-img convert, the deallocated cluster wouldn't be in
>>> the destination.
>>
>> Right. I still think that there has to be an option to keep the image
>> fully allocated. Perhaps what we really need to check is BDRV_O_UNMAP.
> 
> Duh, of course it is.

... and it's handled in bdrv_co_pwrite_zeroes, so Fam's patch should be
okay:

    if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
        flags &= ~BDRV_REQ_MAY_UNMAP;
    }

    return bdrv_co_pwritev(child, offset, count, NULL,
                           BDRV_REQ_ZERO_WRITE | flags);

Paolo

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

[Fam Zheng]

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

---

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 {

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

[Kevin Wolf]

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

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

[Fam Zheng]

On Wed, 09/28 15:04, Fam Zheng wrote:
> Handling this is similar to what is done to the L2 entry in the case of
> compressed clusters.

Kevin, Max, is there anything else I need to do before this patch can be
applied?

Fam

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

[Kevin Wolf]

Am 12.10.2016 um 03:14 hat Fam Zheng geschrieben:
> On Wed, 09/28 15:04, Fam Zheng wrote:
> > Handling this is similar to what is done to the L2 entry in the case of
> > compressed clusters.
> 
> Kevin, Max, is there anything else I need to do before this patch can be
> applied?

Hm, actually, it looks like we had a few discussions, but came to the
conclusions that the patch is alright.

Thanks, applied to the block branch.

Kevin