On 20.10.23 23:56, Andrey Drobyshev wrote:
When zeroizing subclusters within single cluster, detect usage of the
BDRV_REQ_MAY_UNMAP flag and fall through to the subcluster-based discard
operation, much like it's done with the cluster-based discards. That
way subcluster-aligned operations "qemu-io -c 'write -z -u ...'" will
lead to actual unmap.
Ever since the introduction of discard-no-unref, I wonder whether that
is actually an advantage or not. I can’t think of a reason why it’d be
advantageous to drop the allocation.
On the other hand, zero_in_l2_slice() does it indeed, so consistency is
a reasonable argument.
Signed-off-by: Andrey Drobyshev <andrey.drobys...@virtuozzo.com>
---
block/qcow2-cluster.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index cf40f2dc12..040251f2c3 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -2242,7 +2242,7 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t offset,
unsigned nb_subclusters, int flags)
{
BDRVQcow2State *s = bs->opaque;
- uint64_t new_l2_bitmap;
+ uint64_t new_l2_bitmap, l2_bitmap_mask;
int ret, sc = offset_to_sc_index(s, offset);
SubClusterRangeInfo scri = { 0 };
@@ -2251,9 +2251,10 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t offset,
goto out;
}
+ l2_bitmap_mask = QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters);
“l2_bitmap_mask” already wasn’t a great name in patch 4 because it
doesn’t say what mask it is, but in patch 4, there was at least only one
relevant mask. Here, we have two (ALLOC_RANGE and ZERO_RANGE), so the
name should reflect what kind of mask it is.
new_l2_bitmap = scri.l2_bitmap;
- new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters);
- new_l2_bitmap &= ~QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters);
+ new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters);
+ new_l2_bitmap &= ~l2_bitmap_mask;
/*
* If there're no non-zero subclusters left, we might as well zeroize
@@ -2266,6 +2267,16 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t
offset,
1, flags);
}
+ /*
+ * If the request allows discarding subclusters and they're actually
+ * allocated, we go down the discard path since after the discard
+ * operation the subclusters are going to be read as zeroes anyway.
+ */
+ if ((flags & BDRV_REQ_MAY_UNMAP) && (scri.l2_bitmap & l2_bitmap_mask)) {
+ return discard_l2_subclusters(bs, offset, nb_subclusters,
+ QCOW2_DISCARD_REQUEST, false, &scri);
+ }
I wonder why it matters whether any subclusters are allocated, i.e. why
can’t we just immediately use discard_l2_subclusters() whenever
BDRV_REQ_MAY_UNMAP is set, without even fetching SCRI (that would also
save us passing SCRI here, which I’ve already said on patch 4, I don’t
find great).
Doesn’t discard_l2_subclusters() guarantee the clusters read as zero
when full_discard=false? There is this case where it won’t mark the
subclusters as zero if there is no backing file, and none of the
subclusters is allocated. But still, (A) those subclusters will read as
zero, and (B) if there is a problem there, why don’t we just always mark
those subclusters as zero? This optimization only has effect when the
guest discards/zeroes subclusters (not whole clusters) that were already
discarded, so sounds miniscule.
Hanna
+
if (new_l2_bitmap != scri.l2_bitmap) {
set_l2_bitmap(s, scri.l2_slice, scri.l2_index, new_l2_bitmap);
qcow2_cache_entry_mark_dirty(s->l2_table_cache, scri.l2_slice);
