Re: [PATCH] qcow2: add discard-no-unref option

[Hanna Czenczek <hreitz@redhat.com>]

On 15.05.23 09:36, Jean-Louis Dupond wrote:
> When we for example have a sparse qcow2 image and discard: unmap is enabled,
> there can be a lot of fragmentation in the image after some time. Surely on VM's
> that do a lot of writes/deletes.
> This causes the qcow2 image to grow even over 110% of its virtual size,
> because the free gaps in the image get to small to allocate new
> continuous clusters. So it allocates new space as the end of the image.
>
> Disabling discard is not an option, as discard is needed to keep the
> incremental backup size as low as possible. Without discard, the
> incremental backups would become large, as qemu thinks it's just dirty
> blocks but it doesn't know the blocks are empty/useless.
> So we need to avoid fragmentation but also 'empty' the useless blocks in
> the image to have a small incremental backup.
>
> Next to that we also want to send the discards futher down the stack, so
> the underlying blocks are still discarded.
>
> Therefor we introduce a new qcow2 option "discard-no-unref". When
> setting this option to true (defaults to false), the discard requests
> will still be executed, but it will keep the offset of the cluster. And
> it will also pass the discard request further down the stack (if
> discard:unmap is enabled).
> This will avoid fragmentation and for example on a fully preallocated
> qcow2 image, this will make sure the image is perfectly continuous.
>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1621
> Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
> ---
>   block/qcow2-cluster.c  |  16 ++++-
>   block/qcow2-refcount.c | 136 ++++++++++++++++++++++++-----------------
>   block/qcow2.c          |  12 ++++
>   block/qcow2.h          |   3 +
>   qapi/block-core.json   |   4 ++
>   qemu-options.hx        |   6 ++
>   6 files changed, 120 insertions(+), 57 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 39cda7f907..88da70db5e 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1943,10 +1943,22 @@ static int discard_in_l2_slice(BlockDriverState *bs, uint64_t offset,
>               new_l2_entry = new_l2_bitmap = 0;
>           } else if (bs->backing || qcow2_cluster_is_allocated(cluster_type)) {
>               if (has_subclusters(s)) {
> -                new_l2_entry = 0;
> +                if (s->discard_no_unref && (type & QCOW2_DISCARD_REQUEST)) {
> +                    new_l2_entry = old_l2_entry;
> +                } else {
> +                    new_l2_entry = 0;
> +                }
>                   new_l2_bitmap = QCOW_L2_BITMAP_ALL_ZEROES;
>               } else {
> -                new_l2_entry = s->qcow_version >= 3 ? QCOW_OFLAG_ZERO : 0;
> +                if (s->qcow_version >= 3) {
> +                    if (s->discard_no_unref && (type & QCOW2_DISCARD_REQUEST)) {
> +                        new_l2_entry |= QCOW_OFLAG_ZERO;
> +                    } else {
> +                        new_l2_entry = QCOW_OFLAG_ZERO;
> +                    }
> +                } else {
> +                    new_l2_entry = 0;
> +                }
>               }
>           }

Forgot to note something: So this option only works for qcow2 v3. Can we 
have qcow2_update_options_prepare() return an error if s->qcow_version < 
3 and discard_no_unref is set?

Hanna