Re: [PATCH v2 02/11] qcow2: simplify L2 entries accounting for discard-no-unref

[Alexander Ivanov]

On 5/13/24 08:31, Andrey Drobyshev wrote:
> Commits 42a2890a and b2b10904 introduce handling of discard-no-unref
> option in discard_in_l2_slice() and zero_in_l2_slice().  They add even
> more if's when chosing the right l2 entry.  What we really need for this
> option is the new entry simply to contain the same host cluster offset,
> no matter whether we unmap or zeroize the cluster.  For that OR'ing with
> the old entry is enough.
>
> This patch doesn't change the logic and is pure refactoring.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   block/qcow2-cluster.c | 34 +++++++++++++++-------------------
>   1 file changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index ce8c0076b3..5f057ba2fd 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1946,25 +1946,21 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t 
> offset, uint64_t nb_clusters,
>               new_l2_entry = new_l2_bitmap = 0;
>           } else if (bs->backing || qcow2_cluster_is_allocated(cluster_type)) {
>               if (has_subclusters(s)) {
> -                if (keep_reference) {
> -                    new_l2_entry = old_l2_entry;
> -                } else {
> -                    new_l2_entry = 0;
> -                }
> +                new_l2_entry = 0;
>                   new_l2_bitmap = QCOW_L2_BITMAP_ALL_ZEROES;
>               } else {
> -                if (s->qcow_version >= 3) {
> -                    if (keep_reference) {
> -                        new_l2_entry |= QCOW_OFLAG_ZERO;
> -                    } else {
> -                        new_l2_entry = QCOW_OFLAG_ZERO;
> -                    }
> -                } else {
> -                    new_l2_entry = 0;
> -                }
> +                new_l2_entry = s->qcow_version >= 3 ? QCOW_OFLAG_ZERO : 0;
>               }
>           }
>   
> +        /*
> +         * No need to check for the QCOW version since discard-no-unref is
> +         * only allowed since version 3.
> +         */
> +        if (keep_reference) {
> +            new_l2_entry |= old_l2_entry;
> +        }
> +
>           if (old_l2_entry == new_l2_entry && old_l2_bitmap == new_l2_bitmap) {
>               continue;
>           }
> @@ -2064,19 +2060,19 @@ zero_in_l2_slice(BlockDriverState *bs, uint64_t offset,
>               ((flags & BDRV_REQ_MAY_UNMAP) && 
> qcow2_cluster_is_allocated(type));
>           bool keep_reference =
>               (s->discard_no_unref && type != QCOW2_CLUSTER_COMPRESSED);
> -        uint64_t new_l2_entry = old_l2_entry;
> +        uint64_t new_l2_entry = unmap ? 0 : old_l2_entry;
>           uint64_t new_l2_bitmap = old_l2_bitmap;
>   
> -        if (unmap && !keep_reference) {
> -            new_l2_entry = 0;
> -        }
> -
>           if (has_subclusters(s)) {
>               new_l2_bitmap = QCOW_L2_BITMAP_ALL_ZEROES;
>           } else {
>               new_l2_entry |= QCOW_OFLAG_ZERO;
>           }
>   
> +        if (keep_reference) {
> +            new_l2_entry |= old_l2_entry;
> +        }
> +
>           if (old_l2_entry == new_l2_entry && old_l2_bitmap == new_l2_bitmap) {
>               continue;
>           }
Reviewed-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

--
Best regards,
Alexander Ivanov