* [Qemu-devel] [PATCH] qcow2: Set zero flag for discarded clusters @ 2014-02-08 15:28 Kevin Wolf 2014-02-08 16:11 ` Max Reitz 2014-02-08 16:17 ` Max Reitz 0 siblings, 2 replies; 8+ messages in thread From: Kevin Wolf @ 2014-02-08 15:28 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, pbonzini, stefanha, mreitz 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] qcow2: Set zero flag for discarded clusters 2014-02-08 15:28 [Qemu-devel] [PATCH] qcow2: Set zero flag for discarded clusters Kevin Wolf @ 2014-02-08 16:11 ` Max Reitz 2014-02-08 16:17 ` Max Reitz 1 sibling, 0 replies; 8+ messages in thread From: Max Reitz @ 2014-02-08 16:11 UTC (permalink / raw) To: Kevin Wolf, qemu-devel; +Cc: pbonzini, stefanha 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> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] qcow2: Set zero flag for discarded clusters 2014-02-08 15:28 [Qemu-devel] [PATCH] qcow2: Set zero flag for discarded clusters Kevin Wolf 2014-02-08 16:11 ` Max Reitz @ 2014-02-08 16:17 ` Max Reitz 2014-02-08 16:32 ` Kevin Wolf 2014-02-08 16:50 ` [Qemu-devel] [PATCH v2] " Kevin Wolf 1 sibling, 2 replies; 8+ messages in thread From: Max Reitz @ 2014-02-08 16:17 UTC (permalink / raw) To: Kevin Wolf, qemu-devel; +Cc: pbonzini, stefanha 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); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] qcow2: Set zero flag for discarded clusters 2014-02-08 16:17 ` Max Reitz @ 2014-02-08 16:32 ` Kevin Wolf 2014-02-08 16:50 ` [Qemu-devel] [PATCH v2] " Kevin Wolf 1 sibling, 0 replies; 8+ messages in thread From: Kevin Wolf @ 2014-02-08 16:32 UTC (permalink / raw) To: Max Reitz; +Cc: pbonzini, qemu-devel, stefanha 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH v2] qcow2: Set zero flag for discarded clusters 2014-02-08 16:17 ` Max Reitz 2014-02-08 16:32 ` Kevin Wolf @ 2014-02-08 16:50 ` Kevin Wolf 2014-02-14 14:34 ` Stefan Hajnoczi 2014-02-14 17:05 ` Stefan Hajnoczi 1 sibling, 2 replies; 8+ messages in thread From: Kevin Wolf @ 2014-02-08 16:50 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, pbonzini, stefanha, mreitz 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qcow2: Set zero flag for discarded clusters 2014-02-08 16:50 ` [Qemu-devel] [PATCH v2] " Kevin Wolf @ 2014-02-14 14:34 ` Stefan Hajnoczi 2014-02-14 17:05 ` Stefan Hajnoczi 1 sibling, 0 replies; 8+ messages in thread From: Stefan Hajnoczi @ 2014-02-14 14:34 UTC (permalink / raw) To: Kevin Wolf; +Cc: pbonzini, qemu-devel, stefanha, mreitz 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qcow2: Set zero flag for discarded clusters 2014-02-08 16:50 ` [Qemu-devel] [PATCH v2] " Kevin Wolf 2014-02-14 14:34 ` Stefan Hajnoczi @ 2014-02-14 17:05 ` Stefan Hajnoczi 2014-02-14 18:11 ` Kevin Wolf 1 sibling, 1 reply; 8+ messages in thread From: Stefan Hajnoczi @ 2014-02-14 17:05 UTC (permalink / raw) To: Kevin Wolf; +Cc: pbonzini, qemu-devel, stefanha, mreitz 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qcow2: Set zero flag for discarded clusters 2014-02-14 17:05 ` Stefan Hajnoczi @ 2014-02-14 18:11 ` Kevin Wolf 0 siblings, 0 replies; 8+ messages in thread From: Kevin Wolf @ 2014-02-14 18:11 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: pbonzini, qemu-devel, stefanha, mreitz 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-02-14 18:11 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-02-08 15:28 [Qemu-devel] [PATCH] qcow2: Set zero flag for discarded clusters Kevin Wolf 2014-02-08 16:11 ` Max Reitz 2014-02-08 16:17 ` Max Reitz 2014-02-08 16:32 ` Kevin Wolf 2014-02-08 16:50 ` [Qemu-devel] [PATCH v2] " Kevin Wolf 2014-02-14 14:34 ` Stefan Hajnoczi 2014-02-14 17:05 ` Stefan Hajnoczi 2014-02-14 18:11 ` Kevin Wolf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).