On Mon, 2020-11-23 at 19:20 +0100, Alberto Garcia wrote:
> 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.
I also agree, I haven't thought about this.
Best regards,
Maxim Levitsky
>
> Berto
>
