All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
@ 2016-09-28  7:04 Fam Zheng
  2016-09-28 16:11 ` Max Reitz
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Fam Zheng @ 2016-09-28  7:04 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Max Reitz, qemu-block

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
  2016-09-28  7:04 [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP Fam Zheng
@ 2016-09-28 16:11 ` Max Reitz
  2016-09-29  2:21   ` Fam Zheng
  2016-09-29  9:29 ` Kevin Wolf
  2016-10-12  1:14 ` Fam Zheng
  2 siblings, 1 reply; 17+ messages in thread
From: Max Reitz @ 2016-09-28 16:11 UTC (permalink / raw)
  To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, qemu-block

[-- 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 --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
  2016-09-28 16:11 ` Max Reitz
@ 2016-09-29  2:21   ` Fam Zheng
  2016-09-29  7:58     ` Paolo Bonzini
  0 siblings, 1 reply; 17+ messages in thread
From: Fam Zheng @ 2016-09-29  2:21 UTC (permalink / raw)
  To: Max Reitz; +Cc: qemu-devel, Kevin Wolf, qemu-block

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
  2016-09-29  2:21   ` Fam Zheng
@ 2016-09-29  7:58     ` Paolo Bonzini
  2016-09-29  8:10       ` Fam Zheng
  0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2016-09-29  7:58 UTC (permalink / raw)
  To: Fam Zheng, Max Reitz; +Cc: Kevin Wolf, qemu-devel, qemu-block



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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
  2016-09-29  7:58     ` Paolo Bonzini
@ 2016-09-29  8:10       ` Fam Zheng
  2016-10-01 13:08         ` Max Reitz
  0 siblings, 1 reply; 17+ messages in thread
From: Fam Zheng @ 2016-09-29  8:10 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Max Reitz, Kevin Wolf, qemu-devel, qemu-block

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
  2016-09-28  7:04 [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP Fam Zheng
  2016-09-28 16:11 ` Max Reitz
@ 2016-09-29  9:29 ` Kevin Wolf
  2016-09-29  9:55   ` Fam Zheng
  2016-10-12  1:14 ` Fam Zheng
  2 siblings, 1 reply; 17+ messages in thread
From: Kevin Wolf @ 2016-09-29  9:29 UTC (permalink / raw)
  To: Fam Zheng; +Cc: qemu-devel, Max Reitz, qemu-block

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
  2016-09-29  9:29 ` Kevin Wolf
@ 2016-09-29  9:55   ` Fam Zheng
  2016-09-29 10:39     ` Kevin Wolf
  0 siblings, 1 reply; 17+ messages in thread
From: Fam Zheng @ 2016-09-29  9:55 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
  2016-09-29  9:55   ` Fam Zheng
@ 2016-09-29 10:39     ` Kevin Wolf
  2016-09-29 12:14       ` Paolo Bonzini
  2016-09-30  2:04       ` Fam Zheng
  0 siblings, 2 replies; 17+ messages in thread
From: Kevin Wolf @ 2016-09-29 10:39 UTC (permalink / raw)
  To: Fam Zheng; +Cc: qemu-devel, qemu-block, Max Reitz

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
  2016-09-29 10:39     ` Kevin Wolf
@ 2016-09-29 12:14       ` Paolo Bonzini
  2016-09-29 12:48         ` Kevin Wolf
  2016-09-30  2:04       ` Fam Zheng
  1 sibling, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2016-09-29 12:14 UTC (permalink / raw)
  To: Kevin Wolf, Fam Zheng; +Cc: qemu-devel, qemu-block, Max Reitz



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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
  2016-09-29 12:14       ` Paolo Bonzini
@ 2016-09-29 12:48         ` Kevin Wolf
  2016-09-29 12:50           ` Paolo Bonzini
  0 siblings, 1 reply; 17+ messages in thread
From: Kevin Wolf @ 2016-09-29 12:48 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Fam Zheng, qemu-devel, qemu-block, Max Reitz

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
  2016-09-29 12:48         ` Kevin Wolf
@ 2016-09-29 12:50           ` Paolo Bonzini
  2016-09-29 12:56             ` Paolo Bonzini
  0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2016-09-29 12:50 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Fam Zheng, qemu-devel, qemu-block, Max Reitz



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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
  2016-09-29 12:50           ` Paolo Bonzini
@ 2016-09-29 12:56             ` Paolo Bonzini
  0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2016-09-29 12:56 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Fam Zheng, qemu-devel, qemu-block, Max Reitz



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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
  2016-09-29 10:39     ` Kevin Wolf
  2016-09-29 12:14       ` Paolo Bonzini
@ 2016-09-30  2:04       ` Fam Zheng
  2016-09-30 12:38         ` Kevin Wolf
  1 sibling, 1 reply; 17+ messages in thread
From: Fam Zheng @ 2016-09-30  2:04 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz

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 {

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
  2016-09-30  2:04       ` Fam Zheng
@ 2016-09-30 12:38         ` Kevin Wolf
  0 siblings, 0 replies; 17+ messages in thread
From: Kevin Wolf @ 2016-09-30 12:38 UTC (permalink / raw)
  To: Fam Zheng; +Cc: qemu-devel, qemu-block, Max Reitz

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 {

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
  2016-09-29  8:10       ` Fam Zheng
@ 2016-10-01 13:08         ` Max Reitz
  0 siblings, 0 replies; 17+ messages in thread
From: Max Reitz @ 2016-10-01 13:08 UTC (permalink / raw)
  To: Fam Zheng, Paolo Bonzini; +Cc: Kevin Wolf, qemu-devel, qemu-block

[-- 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 --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
  2016-09-28  7:04 [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP Fam Zheng
  2016-09-28 16:11 ` Max Reitz
  2016-09-29  9:29 ` Kevin Wolf
@ 2016-10-12  1:14 ` Fam Zheng
  2016-10-12  8:42   ` Kevin Wolf
  2 siblings, 1 reply; 17+ messages in thread
From: Fam Zheng @ 2016-10-12  1:14 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
  2016-10-12  1:14 ` Fam Zheng
@ 2016-10-12  8:42   ` Kevin Wolf
  0 siblings, 0 replies; 17+ messages in thread
From: Kevin Wolf @ 2016-10-12  8:42 UTC (permalink / raw)
  To: Fam Zheng; +Cc: qemu-devel, qemu-block, Max Reitz

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2016-10-12  8:42 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-28  7:04 [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP Fam Zheng
2016-09-28 16:11 ` Max Reitz
2016-09-29  2:21   ` Fam Zheng
2016-09-29  7:58     ` Paolo Bonzini
2016-09-29  8:10       ` Fam Zheng
2016-10-01 13:08         ` Max Reitz
2016-09-29  9:29 ` Kevin Wolf
2016-09-29  9:55   ` Fam Zheng
2016-09-29 10:39     ` Kevin Wolf
2016-09-29 12:14       ` Paolo Bonzini
2016-09-29 12:48         ` Kevin Wolf
2016-09-29 12:50           ` Paolo Bonzini
2016-09-29 12:56             ` Paolo Bonzini
2016-09-30  2:04       ` Fam Zheng
2016-09-30 12:38         ` Kevin Wolf
2016-10-12  1:14 ` Fam Zheng
2016-10-12  8:42   ` Kevin Wolf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.