| Message ID | 20240513063203.113911-3-andrey.drobyshev@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | qcow2: make subclusters discardable | expand |
On Mon 13 May 2024 09:31:54 AM +03, 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> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
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>
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; }
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(-)