Re: [PATCH v2 09/19] qemu: blockjob: Store list of bitmaps disabled prior to commit

On 3/11/20 7:55 AM, Peter Krempa wrote: I think I get why you don't want them further dirtied, but does it really matter? Visually, consider what happens if we create checkpoint1 after writing data A, then checkpoint2 after writing data B (rendering the first checkpoing read-only) before writing data C, then create an external snapshot (possibly simultaneous with checkpoint3) to create a new active image (the new image also has a distinct bitmap tracking all changes since the snapshot, while the snapshot in the now-backing image remains read-write but unchanged because we no longer write to the backing) before writing data D: backing: AABBCC-- bitmap1: --11---- (disabled) bitmap2: ----11-- (enabled) active: ---D-DD- bitmap3: ---1-11- (enabled) In the above, I used different bitmap names between backing and active (as that is less confusing), but I don't know if that's what your code for bitmaps + external snapshots currently does, or if we end up with two bitmaps both named bitmap2 in both images; but that shouldn't really matter (as qemu needs both the node name and a bitmap name). While in this configuration, a differential backup from checkpoint1 uses backing/bitmap1 + backing/bitmap2 + active/bitmap3 == --11111-; a differential from checkpoint2 uses backing/bitmap2 + active/bitmap3 == ---1111-; a differential from the time of the external snapshot creation uses active/bitmap3 == ---1-11-. So, the question is what happens if we now want to commit active back into backing. If we leave bitmap2 enabled while the commit happens, we end up with: backing: AABDCDD- bitmap1: --11---- (disabled) bitmap2: ---1111- (enabled) You can no longer recreate the point in time where the external snapshot was created (the commit lost checkpoint3), but bitmap2 is still viable for all changes needed for an incremental backup of changes made after bitmap1 was disabled and bitmap2 created. Conversely, if we disable bitmap2 prior to the commit, then you have the option of creating a new bitmap3 prior to the commit to end up with: backing: AABDCDD- bitmap1: --11---- (disabled) bitmap2: ----11-- (disabled) bitmap3: ---1-11- (enabled) such that you now have a point-in-time where you can then do a differential backup from all changes after the external snapshot was created (even though it has now been committed). But your resulting backups from any of the checkpoints see the same cumulative bitmaps as before. And what happens if you accidentally leave bitmap2 enabled during the commit? backing: AABDCDD- bitmap1: --11---- (disabled) bitmap2: ---1111- (disabled) bitmap3: ---1-11- (enabled) You have recorded more bits than strictly necessary in bitmap2, but as bitmap2 is unusable on its own (a differential bitmap HAS to combine bitmap2 with bitmap3, whether you are doing the differential from checkpoint1 or checkoint2), and all of those same bits are recorded in bitamp3, the resulting cumulative bitmap is unchanged. Thus, my question is whether this complexity is necessary because it buys us some safety (is there some failure path I'm overlooking, where keeping bitmap2 unchanged even though backing is partially modified before the failure of a half-completed commit is essential?), but I can still go ahead and review the patch on the assumption that you have a reason for not allowing a stranded-enabled bitmap after a snapshot operation to not be modified during commit. At any rate, I'm not seeing any functional problems with the added code, even if I still have a question on the technical need. Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org