On Fri, May 08, 2015 at 08:29:09AM -0600, Eric Blake wrote: > On 05/08/2015 07:14 AM, Stefan Hajnoczi wrote: > > > No it doesn't. Actions have to appear atomic to the qmp_transaction > > caller. Both approaches achieve that so they are both correct in > > isolation. > > > > The ambiguity is whether "commit the changes" for .commit() means > > "changes take effect" or "discard stashed state, making undo > > impossible". > > > > I think the "discard stashed state, making undo impossible" > > interpretation is good because .commit() is not allowed to fail. That > > function should only do things that never fail. > > > >> That's going to get hard to maintain as we add more transactions. > > > > Yes, we need to be consistent and stick to one of the interpretations in > > order to guarantee ordering. > > > > Unfortunately, there is already an inconsistency: > > > > 1. internal_snapshot - snapshot taken in .prepare() > > 2. external_snapshot - BDS node appended in .commit() > > 3. drive_backup - block job started in .prepare() > > 4. blockdev_backup - block job started in .prepare() > > > > external_snapshot followed by internal_snapshot acts like the reverse > > ordering! > > Is that fatal, though?
Yes, ordering is critical when add-bitmap or clear-bitmap are combined with drive-backup. Typically the drive-backup must happen after add-bitmap or clear-bitmap. There is probably no one who uses external and internal snapshots together in a single 'transaction' command, so my example is contrived but it's the same problem. > Let's see if I'm understanding the problem > correctly: if you start with a.qcow2, then > external_snapshot followed by internal_snapshot > should create b.qcow2 then the internal snapshot inside b.qcow2, while > internal_snapshot followed by external_snapshot > should create the internal snapshot inside a.qcow2, then create b.qcow2 > > But since we create the BDS node later than the internal snapshot is > taken, both sequences currently cause the internal snapshot to live in > a.qcow2. Right. Ordering is not honored :(. Stefan
pgpnHnm6ZKwQL.pgp
Description: PGP signature
