On 10/10/2016 04:57 AM, Kevin Wolf wrote:
Am 07.10.2016 um 20:39 hat John Snow geschrieben:
On 09/30/2016 06:00 PM, John Snow wrote:
Refactor backup_start as backup_job_create, which only creates the job,
but does not automatically start it. The old interface, 'backup_start',
is not kept in favor of limiting the number of nearly-identical iterfaces
(Ah yes, 'iterfaces.')
that would have to be edited to keep up with QAPI changes in the future.
Callers that wish to synchronously start the backup_block_job can
instead just call block_job_start immediately after calling
backup_job_create.
Transactions are updated to use the new interface, calling block_job_start
only during the .commit phase, which helps prevent race conditions where
jobs may finish before we even finish building the transaction. This may
happen, for instance, during empty block backup jobs.
Sadly for me, I realized this patch has a potential problem. When we
were adding the bitmap operations, it became clear that the
atomicity point was during .prepare, not .commit.
e.g. the bitmap is cleared or created during prepare, and backup_run
installs its Write Notifier at that point in time, too.
Strictly speaking that's wrong then.
I agree, though I do remember this coming up during the bitmap review
process that the current point-in-time spot is during prepare at the moment.
I do think that while it's at least a consistent model (The model where
we do in fact commit during .prepare(), and simply undo or revert during
.abort(), and only clean or remove undo-cache in .commit()) it certainly
violates the principle of least surprise and is a little rude...
The write notifier doesn't really hurt because it is never triggered
between prepare and commit (we're holding the lock) and it can just be
removed again.
Clearing the bitmap is a bug because the caller could expect that the
bitmap is in its original state if the transaction fails. I doubt this
is a problem in practice, but we should fix it anyway.
We make a backup to undo the process if it fails. I only mention it to
emphasize that the atomic point appears to be during prepare. In
practice we hold the locks for the whole process, but... I think Paolo
may be actively trying to change that.
By the way, why did we allow to add a 'bitmap' option for DriveBackup
without adding it to BlockdevBackup at the same time?
I don't remember. I'm not sure anyone ever audited it to convince
themselves it was a useful or safe thing to do. I believe at the time I
was pushing for bitmaps in DriveBackup, Fam was still authoring the
BlockdevBackup interface.
By changing BlockJobs to only run on commit, we've severed the
atomicity point such that some actions will take effect during
prepare, and others at commit.
I still think it's the correct thing to do to delay the BlockJobs
until the commit phase, so I will start auditing the code to see how
hard it is to shift the atomicity point to commit instead. If it's
possible to do that, I think from the POV of the managing
application, having the atomicity point be
Feel free to chime in with suggestions and counterpoints until then.
I agree that jobs have to be started only at commit. There may be other
things that are currently happening in prepare that really should be
moved as well, but unless moving one thing but not the other doesn't
break anything that was working, we can fix one thing at a time.
Kevin
Alright, let's give this a whirl.
We have 8 transaction actions:
drive_backup
blockdev_backup
block_dirty_bitmap_add
block_dirty_bitmap_clear
abort
blockdev_snapshot
blockdev_snapshot_sync
blockdev_snapshot_internal_sync
Drive and Blockdev backup are already modified to behave point-in-time
at time of .commit() by changing them to only begin running once the
commit phase occurs.
Bitmap add and clear are trivial to rework; clear just moves the call to
clear in commit, with possibly some action taken to prevent the bitmap
from become used by some other process in the meantime. Add is easy to
rework too, we can create it during prepare but reset it back to zero
during commit if necessary.
Abort needs no changes.
blockdev_snapshot[_sync] actually appears to already be doing the right
thing, by only installing the new top layer during commit, which makes
this action inconsistent by current semantics, but requires no changes
to move to the desired new semantics.
That leaves only the internal snapshot to worry about, which does
admittedly look like quite the yak to shave. It's a bit out of scope for
me, but Kevin, do you think this is possible?
Looks like implementations are qcow2, rbd, and sheepdog. I imagine this
would need to be split into prepare and commit semantics to accommodate
this change... though we don't have any meaningful control over the rbd
implementation.
Any thoughts? I could conceivably just change everything over to working
primarily during .commit(), and just argue that the locks held for the
transaction are sufficient to leave the internal snapshot alone "for
now," ...
--js
