block: prohibit migrations during tasks

requires:
[PATCH v2] migration: disallow migrate_add_blocker during migration

We don't want to allow migrations during sensitive operations such as snapshots or mirroring. In conjunction with the previous patch, we will also prohibit the user from starting any block jobs while migrations are active.

Questions:
- Are there other actions that need to be guarded?
- Are there actions here that are guarded, but should not be?
- Is this worth doing at all? libvirt saves us in most cases.
- What other cases besides a fully synchronized mirror might be valid in a migration workflow?

Known open issues:
- Does not guard against incoming migrations, only outgoing ones. Is this a problem? Are there valid use cases for running jobs on a machine before or during an incoming migration?

John Snow (3):
block: prohibit migration during BlockJobs block/mirror: allow migration after sync
block: prohibit migration during transactions

 block/mirror.c           |  2 ++
 blockdev.c               | 12 ++++++++++++
 blockjob.c               | 16 ++++++++++++++++
 include/block/blockjob.h |  8 ++++++++
 4 files changed, 38 insertions(+)

2.4.3

Unless we can prove this to be safe for specific cases, the default should be to prohibit migration during BlockJobs.

In conjunction with
"migration: disallow_migrate_add_blocker during migration", this should be sufficient to disallow the blockjob from starting in the event of an in-progress migration.

---
 blockjob.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

index 62bb906..b5849b3 100644
@@ -35,12 +35,14 @@
 #include "qmp-commands.h"
 #include "qemu/timer.h"
 #include "qapi-event.h"
+#include "migration/migration.h"
 
 void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
                        int64_t speed, BlockCompletionFunc *cb,
                        void *opaque, Error **errp)
 {
     BlockJob *job;
+    int ret;
 
     if (bs->job) {
         error_setg(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));

@@ -71,6 +73,14 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
             return NULL;
         }
     }
+
+    /* Re-use the op-blocker as a migration blocker */
+    ret = migrate_add_blocker(job->blocker, errp);
+    if (ret < 0) {
+        block_job_release(bs);
+        return NULL;
+    }
+
     return job;
 }
 

@@ -80,6 +90,7 @@ void block_job_release(BlockDriverState *bs)
 
     bs->job = NULL;
     bdrv_op_unblock_all(bs, job->blocker);
+    migrate_del_blocker(job->blocker);
     error_free(job->blocker);
     g_free(job);
 }

2.4.3

Block jobs do not affect the current block, only other block device, hence they *are* safe for migration.

What you want, I think, is the target not to be garbage when migration ends. Based on this you can block specific cases, namely mirror which you already do allow (patch 2) and backup except for sync='none'.

Paolo

Can you elaborate for me here?

It would be nice if the target wasn't garbage, yes :)

I allow mirror in specific circumstances -- you can't start a mirror, but if an existing mirror has hit the sync phase, that's OK.

I can try to do a more exhaustive audit of what should and should not work, but my thought was "guilty before proven innocent."

Am 02.10.2015 um 20:17 hat John Snow geschrieben:

I think I've come to the conclusion that qemu can't even know in all cases (particularly mirroring) because it depends on what the resulting image is used for.

If we mirror in order to do a live storage migration, then obviously it needs to run during migration. In this case, your restriction "only in sync phase" seems to be right.

If we mirror for some other reason, the mirroring job should probably continue running on the destination host. The management tool can set up the destination accordingly, but if it doesn't, the job is silently cancelled. The point is that qemu (even more on the source host) doesn't know which case is true, so rejecting the migration seems wrong.

If we do commit or stream, the job is silently cancelled and must be restarted on the destination host. It's the same case as above.

All (or most) of the restarting jobs on the destination doesn't come for free, we usually repeat some useless work that the source host had already done. That's not a reason to reject migrations, just cost for the user to consider before they start migration.

So in the end, I'm not sure how much qemu can actually do here.

Kevin

Migration should be blocked if it causes e.g. data corruption or guest failures.

However, a block job started on BDS bds0 does _not_ modify the data on bds0. It does one of the following:

1) modifies bds0 without changing the data that guests see (example:
streaming)

2) modifies a block device in bds0's backing chain without changing the data that the guests see in bds0 (example: commit)

3) modifies another block device unrelated to bds0 (example: mirror, backup)

Doing this across migration requires some care (and there is a bug; see a patch I'll send soon) but generally works.

It would also be acceptable if it was, though, as long as following the proper protocol (e.g. wait for the sync phase before triggering migration) never makes the target garbage.

For (1) and (2) there's no way to be guilty. For (3)... well, there's just two block jobs in that category and you do cover one of them; if we want to block migration in some cases, doing it for the remaining one is not a big request. :)

Paolo

After the mirroring blockjob reaches synchronization, allow migration to resume.

---
 block/mirror.c           | 2 ++
 blockjob.c               | 5 +++++
 include/block/blockjob.h | 8 ++++++++
 3 files changed, 15 insertions(+)

index a258926..5eb469a 100644
@@ -415,6 +415,7 @@ static void coroutine_fn mirror_run(void *opaque)
         /* Report BLOCK_JOB_READY and wait for complete. */
         block_job_event_ready(&s->common);
         s->synced = true;
+        block_job_allow_migration(&s->common);
         while (!block_job_is_cancelled(&s->common) && !s->should_complete) {
             block_job_yield(&s->common);
         }

@@ -540,6 +541,7 @@ static void coroutine_fn mirror_run(void *opaque)
                 if (!s->synced) {
                     block_job_event_ready(&s->common);
                     s->synced = true;
+                    block_job_allow_migration(&s->common);
                 }
 
                 should_complete = s->should_complete ||

index b5849b3..bcb4fca 100644
@@ -95,6 +95,11 @@ void block_job_release(BlockDriverState *bs)
     g_free(job);
 }
 
+void block_job_allow_migration(BlockJob *job)
+{
+    migrate_del_blocker(job->blocker);
+}
+
 void block_job_completed(BlockJob *job, int ret)
 {
     BlockDriverState *bs = job->bs;

index dd9d5e6..2278484 100644
@@ -147,6 +147,14 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
                        void *opaque, Error **errp);
 
 /**
+ * block_job_allow_migration:
+ * @job: The job to modify to lift its restriction on migrations.
+ *
+ * Lift this job's restriction on prohibiting outgoing migrations.
+ */
+void block_job_allow_migration(BlockJob *job);
+
+/**
  * block_job_sleep_ns:
  * @job: The job that calls the function.
  * @clock: The clock to sleep on.

2.4.3

Similarly to BlockJobs, prohibit migration at least during the synchronous phase of a transaction.

In particular, this guards internal and external snapshots, which are implemented via transaction actions through blockdev_do_action.

---
 blockdev.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

index 32b04b4..231dcf6 100644
@@ -49,6 +49,7 @@
 #include "qmp-commands.h"
 #include "trace.h"
 #include "sysemu/arch_init.h"
+#include "migration/migration.h"
 
 static const char *const if_name[IF_COUNT] = {
     [IF_NONE] = "none",

@@ -1759,6 +1760,14 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
     TransactionActionList *dev_entry = dev_list;
     BlkTransactionState *state, *next;
     Error *local_err = NULL;
+    Error *blocker = NULL;
+    int ret;
+
+    error_setg(&blocker, "Block device(s) are in use by a Block Transaction");
+    ret = migrate_add_blocker(blocker, errp);
+    if (ret < 0) {
+        goto cleanup_mig;
+    }
 
     QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionState) snap_bdrv_states;
     QSIMPLEQ_INIT(&snap_bdrv_states);

@@ -1814,6 +1823,9 @@ exit:
         }
         g_free(state);
     }
+ cleanup_mig:
+    migrate_del_blocker(blocker);
+    error_free(blocker);
 }
 
 

2.4.3

Well, it definitely can't now ! :)

This is actually more for the other case: The migration starts, and we want to prohibit snapshots and transactions during that period. Together with the patch this depends on, we accomplish that.

The workflow of snapshot/transaction-before-migration isn't currently possible.