17.03.2020 21:16, Alberto Garcia wrote:
Two changes are needed in this function:
1) A full discard deallocates a cluster so we can skip the operation if
it is already unallocated. With extended L2 entries however if any
of the subclusters has the 'all zeroes' bit set then we have to
clear it.
2) Setting the QCOW_OFLAG_ZERO bit of the L2 entry is forbidden if an
image has extended L2 entries. Instead, the individual 'all zeroes'
bits must be used.
Signed-off-by: Alberto Garcia <be...@igalia.com>
---
block/qcow2-cluster.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 746006a117..824c710760 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1790,12 +1790,20 @@ static int discard_in_l2_slice(BlockDriverState *bs,
uint64_t offset,
* TODO We might want to use bdrv_block_status(bs) here, but we're
* holding s->lock, so that doesn't work today.
*
- * If full_discard is true, the sector should not read back as zeroes,
+ * If full_discard is true, the cluster should not read back as zeroes,
* but rather fall through to the backing file.
*/
switch (qcow2_get_cluster_type(bs, old_l2_entry)) {
case QCOW2_CLUSTER_UNALLOCATED:
- if (full_discard || !bs->backing) {
+ if (full_discard) {
+ /* If the image has extended L2 entries we can only
+ * skip this operation if the L2 bitmap is zero. */
+ uint64_t bitmap = has_subclusters(s) ?
+ get_l2_bitmap(s, l2_slice, l2_index + i) : 0;
+ if (bitmap == 0) {
+ continue;
+ }
+ } else if (!bs->backing) {
continue;
}
Hmm, so you do continue if full_discard is false AND bitmap != 0 & !bs->backing,
but you do not continue if full_discard is true AND bitmap != 0 & !bs->backing (as you
will not go to "else") branch.
Seems it's a mistake.
I think, correct condition is
if (!bs->backing || full_discard && !get_l2_bitmap(s, l2_slice, l2_index + i))
, but, for doing so we also need
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -565,6 +565,7 @@ static inline uint64_t get_l2_entry(BDRVQcow2State *s,
uint64_t *l2_slice,
return be64_to_cpu(l2_slice[idx]);
}
+/* Return l2-entry bitmap if image has subclusters and 0 otherwise. */
static inline uint64_t get_l2_bitmap(BDRVQcow2State *s, uint64_t *l2_slice,
int idx)
{
@@ -572,7 +573,6 @@ static inline uint64_t get_l2_bitmap(BDRVQcow2State *s,
uint64_t *l2_slice,
idx *= l2_entry_size(s) / sizeof(uint64_t);
return be64_to_cpu(l2_slice[idx + 1]);
} else {
- /* For convenience only; the caller should ignore this value. */
return 0;
}
}
or if you don't want, keep it explicit
if (!bs->backing || full_discard && (!has_subclusters(s) || !get_l2_bitmap(s,
l2_slice, l2_index + i)))
=====
In case QCOW2_CLUSTER_ZERO_PLAIN, worth assert !has_subclusters(s) or mark
image corrupted ?
break;
@@ -1817,7 +1825,11 @@ static int discard_in_l2_slice(BlockDriverState *bs,
uint64_t offset,
/* First remove L2 entries */
qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
- if (!full_discard && s->qcow_version >= 3) {
+ if (has_subclusters(s)) {
+ set_l2_entry(s, l2_slice, l2_index + i, 0);
+ set_l2_bitmap(s, l2_slice, l2_index + i,
+ full_discard ? 0 : QCOW_L2_BITMAP_ALL_ZEROES);
+ } else if (!full_discard && s->qcow_version >= 3) {
set_l2_entry(s, l2_slice, l2_index + i, QCOW_OFLAG_ZERO);
} else {
set_l2_entry(s, l2_slice, l2_index + i, 0);
--
Best regards,
Vladimir