[v2] qcow2: Set zero flag for discarded clusters

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)
Failures: 046
Failed 1 of 1 tests

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