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