[Qemu-devel] [PATCH] qcow2: Set zero flag for discarded clusters

[Qemu-devel] [PATCH] qcow2: Set zero flag for discarded clusters

[Kevin Wolf]

Instead of making the backing file contents visible again after a discard
request, set the zero flag if possible (i.e. on version >= 3).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 25d45d1..008bc04 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1333,13 +1333,31 @@ static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
         uint64_t old_offset;
 
         old_offset = be64_to_cpu(l2_table[l2_index + i]);
-        if ((old_offset & L2E_OFFSET_MASK) == 0) {
+
+        /*
+         * Make sure that a discarded area reads back as zeroes for v3 images
+         * (we cannot do it for v2 without actually writing a zero-filled
+         * buffer). We can skip the operation if the cluster is already marked
+         * as zero, or if it's unallocated and we don't have a backing file.
+         *
+         * TODO We might want to use bdrv_get_block_status(bs) here, but we're
+         * holding s->lock, so that doesn't work today.
+         */
+        if (!!(old_offset & QCOW_OFLAG_ZERO)) {
+            continue;
+        }
+
+        if ((old_offset & L2E_OFFSET_MASK) == 0 && !bs->backing_hd) {
             continue;
         }
 
         /* First remove L2 entries */
         qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
-        l2_table[l2_index + i] = cpu_to_be64(0);
+        if (s->version >= 3) {
+            l2_table[l2_index + i] = cpu_to_be64(QCOW_OFLAG_ZERO);
+        } else {
+            l2_table[l2_index + i] = cpu_to_be64(0);
+        }
 
         /* Then decrease the refcount */
         qcow2_free_any_clusters(bs, old_offset, 1, type);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] qcow2: Set zero flag for discarded clusters

[Max Reitz]

On 08.02.2014 16:28, Kevin Wolf wrote:
> Instead of making the backing file contents visible again after a discard
> request, set the zero flag if possible (i.e. on version >= 3).
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/qcow2-cluster.c | 22 ++++++++++++++++++++--
>   1 file changed, 20 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH] qcow2: Set zero flag for discarded clusters

[Max Reitz]

On 08.02.2014 16:28, Kevin Wolf wrote:
> Instead of making the backing file contents visible again after a discard
> request, set the zero flag if possible (i.e. on version >= 3).
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/qcow2-cluster.c | 22 ++++++++++++++++++++--
>   1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 25d45d1..008bc04 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1333,13 +1333,31 @@ static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
>           uint64_t old_offset;
>   
>           old_offset = be64_to_cpu(l2_table[l2_index + i]);
> -        if ((old_offset & L2E_OFFSET_MASK) == 0) {
> +
> +        /*
> +         * Make sure that a discarded area reads back as zeroes for v3 images
> +         * (we cannot do it for v2 without actually writing a zero-filled
> +         * buffer). We can skip the operation if the cluster is already marked
> +         * as zero, or if it's unallocated and we don't have a backing file.
> +         *
> +         * TODO We might want to use bdrv_get_block_status(bs) here, but we're
> +         * holding s->lock, so that doesn't work today.
> +         */
> +        if (!!(old_offset & QCOW_OFLAG_ZERO)) {
> +            continue;
> +        }
> +
> +        if ((old_offset & L2E_OFFSET_MASK) == 0 && !bs->backing_hd) {
>               continue;
>           }
>   
>           /* First remove L2 entries */
>           qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
> -        l2_table[l2_index + i] = cpu_to_be64(0);
> +        if (s->version >= 3) {

Oh, I revoke my reviewed-by from earlier. This should probably be 
"s->qcow_version"; otherwise, it won't compile.

Max

> +            l2_table[l2_index + i] = cpu_to_be64(QCOW_OFLAG_ZERO);
> +        } else {
> +            l2_table[l2_index + i] = cpu_to_be64(0);
> +        }
>   
>           /* Then decrease the refcount */
>           qcow2_free_any_clusters(bs, old_offset, 1, type);

Re: [Qemu-devel] [PATCH] qcow2: Set zero flag for discarded clusters

[Kevin Wolf]

Am 08.02.2014 um 17:17 hat Max Reitz geschrieben:
> On 08.02.2014 16:28, Kevin Wolf wrote:
> >Instead of making the backing file contents visible again after a discard
> >request, set the zero flag if possible (i.e. on version >= 3).
> >
> >Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >---
> >  block/qcow2-cluster.c | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> >diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> >index 25d45d1..008bc04 100644
> >--- a/block/qcow2-cluster.c
> >+++ b/block/qcow2-cluster.c
> >@@ -1333,13 +1333,31 @@ static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
> >          uint64_t old_offset;
> >          old_offset = be64_to_cpu(l2_table[l2_index + i]);
> >-        if ((old_offset & L2E_OFFSET_MASK) == 0) {
> >+
> >+        /*
> >+         * Make sure that a discarded area reads back as zeroes for v3 images
> >+         * (we cannot do it for v2 without actually writing a zero-filled
> >+         * buffer). We can skip the operation if the cluster is already marked
> >+         * as zero, or if it's unallocated and we don't have a backing file.
> >+         *
> >+         * TODO We might want to use bdrv_get_block_status(bs) here, but we're
> >+         * holding s->lock, so that doesn't work today.
> >+         */
> >+        if (!!(old_offset & QCOW_OFLAG_ZERO)) {
> >+            continue;
> >+        }
> >+
> >+        if ((old_offset & L2E_OFFSET_MASK) == 0 && !bs->backing_hd) {
> >              continue;
> >          }
> >          /* First remove L2 entries */
> >          qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
> >-        l2_table[l2_index + i] = cpu_to_be64(0);
> >+        if (s->version >= 3) {
> 
> Oh, I revoke my reviewed-by from earlier. This should probably be
> "s->qcow_version"; otherwise, it won't compile.

Indeed. Makes we wonder what source it was that I tested... :-/

Kevin

[Qemu-devel] [PATCH v2] qcow2: Set zero flag for discarded clusters

[Kevin Wolf]

Instead of making the backing file contents visible again after a discard
request, set the zero flag if possible (i.e. on version >= 3).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 25d45d1..9461969 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1333,13 +1333,31 @@ static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
         uint64_t old_offset;
 
         old_offset = be64_to_cpu(l2_table[l2_index + i]);
-        if ((old_offset & L2E_OFFSET_MASK) == 0) {
+
+        /*
+         * Make sure that a discarded area reads back as zeroes for v3 images
+         * (we cannot do it for v2 without actually writing a zero-filled
+         * buffer). We can skip the operation if the cluster is already marked
+         * as zero, or if it's unallocated and we don't have a backing file.
+         *
+         * TODO We might want to use bdrv_get_block_status(bs) here, but we're
+         * holding s->lock, so that doesn't work today.
+         */
+        if (!!(old_offset & QCOW_OFLAG_ZERO)) {
+            continue;
+        }
+
+        if ((old_offset & L2E_OFFSET_MASK) == 0 && !bs->backing_hd) {
             continue;
         }
 
         /* First remove L2 entries */
         qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
-        l2_table[l2_index + i] = cpu_to_be64(0);
+        if (s->qcow_version >= 3) {
+            l2_table[l2_index + i] = cpu_to_be64(QCOW_OFLAG_ZERO);
+        } else {
+            l2_table[l2_index + i] = cpu_to_be64(0);
+        }
 
         /* Then decrease the refcount */
         qcow2_free_any_clusters(bs, old_offset, 1, type);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2] qcow2: Set zero flag for discarded clusters

[Stefan Hajnoczi]

On Sat, Feb 08, 2014 at 05:50:02PM +0100, Kevin Wolf wrote:
> Instead of making the backing file contents visible again after a discard
> request, set the zero flag if possible (i.e. on version >= 3).
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cluster.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)

Thanks, applied v2 to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

Re: [Qemu-devel] [PATCH v2] qcow2: Set zero flag for discarded clusters

[Stefan Hajnoczi]

On Sat, Feb 08, 2014 at 05:50:02PM +0100, Kevin Wolf wrote:
> Instead of making the backing file contents visible again after a discard
> request, set the zero flag if possible (i.e. on version >= 3).
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cluster.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 25d45d1..9461969 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1333,13 +1333,31 @@ static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
>          uint64_t old_offset;
>  
>          old_offset = be64_to_cpu(l2_table[l2_index + i]);
> -        if ((old_offset & L2E_OFFSET_MASK) == 0) {
> +
> +        /*
> +         * Make sure that a discarded area reads back as zeroes for v3 images
> +         * (we cannot do it for v2 without actually writing a zero-filled
> +         * buffer). We can skip the operation if the cluster is already marked
> +         * as zero, or if it's unallocated and we don't have a backing file.
> +         *
> +         * TODO We might want to use bdrv_get_block_status(bs) here, but we're
> +         * holding s->lock, so that doesn't work today.
> +         */
> +        if (!!(old_offset & QCOW_OFLAG_ZERO)) {
> +            continue;
> +        }
> +
> +        if ((old_offset & L2E_OFFSET_MASK) == 0 && !bs->backing_hd) {
>              continue;
>          }
>  
>          /* First remove L2 entries */
>          qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
> -        l2_table[l2_index + i] = cpu_to_be64(0);
> +        if (s->qcow_version >= 3) {
> +            l2_table[l2_index + i] = cpu_to_be64(QCOW_OFLAG_ZERO);
> +        } else {
> +            l2_table[l2_index + i] = cpu_to_be64(0);
> +        }
>  
>          /* Then decrease the refcount */
>          qcow2_free_any_clusters(bs, old_offset, 1, type);

Oops, this breaks qemu-iotests 046.  I have dropped it from the pull request.

046 1s ... - output mismatch (see 046.out.bad)
--- 046.out	2014-02-03 16:06:23.917196159 +0100
+++ 046.out.bad	2014-02-14 18:02:01.577193856 +0100
@@ -187,10 +187,12 @@
 24 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 8192/8192 bytes at offset 516096
 8 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Pattern verification failed at offset 524288, 24576 bytes
 read 24576/24576 bytes at offset 524288
 24 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 8192/8192 bytes at offset 548864
 8 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Pattern verification failed at offset 557056, 8192 bytes
 read 8192/8192 bytes at offset 557056
 8 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 57344/57344 bytes at offset 565248
@@ -199,10 +201,12 @@
 24 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 8192/8192 bytes at offset 647168
 8 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Pattern verification failed at offset 655360, 24576 bytes
 read 24576/24576 bytes at offset 655360
 24 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 8192/8192 bytes at offset 679936
 8 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Pattern verification failed at offset 688128, 8192 bytes
 read 8192/8192 bytes at offset 688128
 8 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 57344/57344 bytes at offset 696320
Failures: 046
Failed 1 of 1 tests

Re: [Qemu-devel] [PATCH v2] qcow2: Set zero flag for discarded clusters

[Kevin Wolf]

Am 14.02.2014 um 18:05 hat Stefan Hajnoczi geschrieben:
> On Sat, Feb 08, 2014 at 05:50:02PM +0100, Kevin Wolf wrote:
> > Instead of making the backing file contents visible again after a discard
> > request, set the zero flag if possible (i.e. on version >= 3).
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  block/qcow2-cluster.c | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > index 25d45d1..9461969 100644
> > --- a/block/qcow2-cluster.c
> > +++ b/block/qcow2-cluster.c
> > @@ -1333,13 +1333,31 @@ static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
> >          uint64_t old_offset;
> >  
> >          old_offset = be64_to_cpu(l2_table[l2_index + i]);
> > -        if ((old_offset & L2E_OFFSET_MASK) == 0) {
> > +
> > +        /*
> > +         * Make sure that a discarded area reads back as zeroes for v3 images
> > +         * (we cannot do it for v2 without actually writing a zero-filled
> > +         * buffer). We can skip the operation if the cluster is already marked
> > +         * as zero, or if it's unallocated and we don't have a backing file.
> > +         *
> > +         * TODO We might want to use bdrv_get_block_status(bs) here, but we're
> > +         * holding s->lock, so that doesn't work today.
> > +         */
> > +        if (!!(old_offset & QCOW_OFLAG_ZERO)) {
> > +            continue;
> > +        }
> > +
> > +        if ((old_offset & L2E_OFFSET_MASK) == 0 && !bs->backing_hd) {
> >              continue;
> >          }
> >  
> >          /* First remove L2 entries */
> >          qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
> > -        l2_table[l2_index + i] = cpu_to_be64(0);
> > +        if (s->qcow_version >= 3) {
> > +            l2_table[l2_index + i] = cpu_to_be64(QCOW_OFLAG_ZERO);
> > +        } else {
> > +            l2_table[l2_index + i] = cpu_to_be64(0);
> > +        }
> >  
> >          /* Then decrease the refcount */
> >          qcow2_free_any_clusters(bs, old_offset, 1, type);
> 
> Oops, this breaks qemu-iotests 046.  I have dropped it from the pull request.

Okay. This is kind of nasty, because we now have different expected
results for v2 and v3 images. I'll have to see how the test case is
fixed in the best way. Perhaps it's best not to check discarded areas at
all, because strictly speaking they are undefined.

Kevin

Re: [Qemu-devel] [PATCH v2] qcow2: Set zero flag for discarded clusters

[Kevin Wolf]

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

Am 17.02.2014 um 18:12 hat Eric Blake geschrieben:
> On 02/17/2014 07:45 AM, Kevin Wolf wrote:
> > Instead of making the backing file contents visible again after a discard
> > request, set the zero flag if possible (i.e. on version >= 3).
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qcow2-cluster.c  | 22 ++++++++++++++++++++--
> >  tests/qemu-iotests/046 | 18 ++++++++++++++----
> >  2 files changed, 34 insertions(+), 6 deletions(-)
> > 
> 
> > +        if (!!(old_offset & QCOW_OFLAG_ZERO)) {
> 
> The !! is not necessary here; any non-zero value in a boolean context
> gives the same result as an explicit conversion to 0-or-1.

Thanks, that looks a bit odd indeed. I'll change it.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2] qcow2: Set zero flag for discarded clusters

[Eric Blake]

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

On 02/17/2014 07:45 AM, Kevin Wolf wrote:
> Instead of making the backing file contents visible again after a discard
> request, set the zero flag if possible (i.e. on version >= 3).
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2-cluster.c  | 22 ++++++++++++++++++++--
>  tests/qemu-iotests/046 | 18 ++++++++++++++----
>  2 files changed, 34 insertions(+), 6 deletions(-)
> 

> +        if (!!(old_offset & QCOW_OFLAG_ZERO)) {

The !! is not necessary here; any non-zero value in a boolean context
gives the same result as an explicit conversion to 0-or-1.

But as that's cosmetic, I'm okay whether you leave it as is or simplify
it, when adding:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

[Qemu-devel] [PATCH v2] qcow2: Set zero flag for discarded clusters

[Kevin Wolf]

Instead of making the backing file contents visible again after a discard
request, set the zero flag if possible (i.e. on version >= 3).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c  | 22 ++++++++++++++++++++--
 tests/qemu-iotests/046 | 18 ++++++++++++++----
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index cd2dcfa..f404ef1 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1332,13 +1332,31 @@ static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
         uint64_t old_offset;
 
         old_offset = be64_to_cpu(l2_table[l2_index + i]);
-        if ((old_offset & L2E_OFFSET_MASK) == 0) {
+
+        /*
+         * Make sure that a discarded area reads back as zeroes for v3 images
+         * (we cannot do it for v2 without actually writing a zero-filled
+         * buffer). We can skip the operation if the cluster is already marked
+         * as zero, or if it's unallocated and we don't have a backing file.
+         *
+         * TODO We might want to use bdrv_get_block_status(bs) here, but we're
+         * holding s->lock, so that doesn't work today.
+         */
+        if (!!(old_offset & QCOW_OFLAG_ZERO)) {
+            continue;
+        }
+
+        if ((old_offset & L2E_OFFSET_MASK) == 0 && !bs->backing_hd) {
             continue;
         }
 
         /* First remove L2 entries */
         qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
-        l2_table[l2_index + i] = cpu_to_be64(0);
+        if (s->qcow_version >= 3) {
+            l2_table[l2_index + i] = cpu_to_be64(QCOW_OFLAG_ZERO);
+        } else {
+            l2_table[l2_index + i] = cpu_to_be64(0);
+        }
 
         /* Then decrease the refcount */
         qcow2_free_any_clusters(bs, old_offset, 1, type);
diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046
index 2d44bbb..e0be46c 100755
--- a/tests/qemu-iotests/046
+++ b/tests/qemu-iotests/046
@@ -193,6 +193,16 @@ echo "== Verify image content =="
 
 function verify_io()
 {
+    if ($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep "compat: 0.10" > /dev/null); then
+        # For v2 images, discarded clusters are read from the backing file
+        # Keep the variable empty so that the backing file value can be used as
+        # the default below
+        discarded=
+    else
+        # Discarded clusters are zeroed for v3 or later
+        discarded=0
+    fi
+
     echo read -P 0 0 0x10000
 
     echo read -P 1  0x10000 0x2000
@@ -221,16 +231,16 @@ function verify_io()
     echo read -P 70 0x78000 0x6000
     echo read -P 7  0x7e000 0x2000
 
-    echo read -P 8  0x80000 0x6000
+    echo read -P ${discarded:-8} 0x80000 0x6000
     echo read -P 80 0x86000 0x2000
-    echo read -P 8  0x88000 0x2000
+    echo read -P ${discarded:-8} 0x88000 0x2000
     echo read -P 81 0x8a000 0xe000
     echo read -P 90 0x98000 0x6000
     echo read -P 9  0x9e000 0x2000
 
-    echo read -P 10  0xa0000 0x6000
+    echo read -P ${discarded:-10} 0xa0000 0x6000
     echo read -P 100 0xa6000 0x2000
-    echo read -P 10  0xa8000 0x2000
+    echo read -P ${discarded:-10} 0xa8000 0x2000
     echo read -P 101 0xaa000 0xe000
     echo read -P 110 0xb8000 0x8000
 
-- 
1.8.1.4