On Mon 23 Nov 2020 05:09:41 PM CET, Kevin Wolf wrote:
>> Commit 205fa50750 ("qcow2: Add subcluster support to zero_in_l2_slice()")
>> introduced a subtle change to code in zero_in_l2_slice:
>>
>> It swapped the order of
>>
>> 1. qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
>> 2. set_l2_entry(s, l2_slice, l2_index + i, QCOW_OFLAG_ZERO);
>> 3. qcow2_free_any_clusters(bs, old_offset, 1, QCOW2_DISCARD_REQUEST);
>>
>> To
>>
>> 1. qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
>> 2. qcow2_free_any_clusters(bs, old_offset, 1, QCOW2_DISCARD_REQUEST);
>> 3. set_l2_entry(s, l2_slice, l2_index + i, QCOW_OFLAG_ZERO);
Ouch :( Good catch!
>> - qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
>> if (unmap) {
>> qcow2_free_any_cluster(bs, old_l2_entry, QCOW2_DISCARD_REQUEST);
>> }
>> set_l2_entry(s, l2_slice, l2_index + i, new_l2_entry);
>> + qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
>
> Good catch, but I think your order is wrong, too. We need the original
> order from before 205fa50750:
>
> 1. qcow2_cache_entry_mark_dirty()
> set_l2_entry() + set_l2_bitmap()
>
> 2. qcow2_free_any_cluster()
I agree with Kevin on this.
Berto
