[Qemu-devel] [PATCH 0/3] block: prohibit migrations during tasks

[Qemu-devel] [PATCH 0/3] block: prohibit migrations during tasks

[John Snow]

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

[Qemu-devel] [PATCH 1/3] block: prohibit migration during BlockJobs

[John Snow]

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.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockjob.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/blockjob.c b/blockjob.c
index 62bb906..b5849b3 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -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

Re: [Qemu-devel] [PATCH 1/3] block: prohibit migration during BlockJobs

[Paolo Bonzini]

On 01/10/2015 18:34, John Snow wrote:
> Unless we can prove this to be safe for specific cases,
> the default should be to prohibit migration during BlockJobs.

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

> 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.

Re: [Qemu-devel] [PATCH 1/3] block: prohibit migration during BlockJobs

[John Snow]

On 10/01/2015 02:03 PM, Paolo Bonzini wrote:
> 
> 
> On 01/10/2015 18:34, John Snow wrote:
>> Unless we can prove this to be safe for specific cases,
>> the default should be to prohibit migration during BlockJobs.
> 
> Block jobs do not affect the current block, only other block device,
> hence they *are* safe for migration.
> 

Can you elaborate for me here?

> 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
> 

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."

>> 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.
> 

Re: [Qemu-devel] [PATCH 1/3] block: prohibit migration during BlockJobs

[Kevin Wolf]

Am 02.10.2015 um 20:17 hat John Snow geschrieben:
> On 10/01/2015 02:03 PM, Paolo Bonzini wrote:
> > On 01/10/2015 18:34, John Snow wrote:
> >> Unless we can prove this to be safe for specific cases,
> >> the default should be to prohibit migration during BlockJobs.
> > 
> > Block jobs do not affect the current block, only other block device,
> > hence they *are* safe for migration.
> > 
> 
> Can you elaborate for me here?
> 
> > 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
> 
> 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."

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

Re: [Qemu-devel] [PATCH 1/3] block: prohibit migration during BlockJobs

[Paolo Bonzini]

On 02/10/2015 20:17, John Snow wrote:
> 
> 
> On 10/01/2015 02:03 PM, Paolo Bonzini wrote:
>>
>>
>> On 01/10/2015 18:34, John Snow wrote:
>>> Unless we can prove this to be safe for specific cases,
>>> the default should be to prohibit migration during BlockJobs.
>>
>> Block jobs do not affect the current block, only other block device,
>> hence they *are* safe for migration.
> 
> Can you elaborate for me here?

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.

>> 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'.
> 
> It would be nice if the target wasn't garbage, yes :)

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.

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

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

[Qemu-devel] [PATCH 2/3] block/mirror: allow migration after sync

[John Snow]

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

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/mirror.c           | 2 ++
 blockjob.c               | 5 +++++
 include/block/blockjob.h | 8 ++++++++
 3 files changed, 15 insertions(+)

diff --git a/block/mirror.c b/block/mirror.c
index a258926..5eb469a 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -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 ||
diff --git a/blockjob.c b/blockjob.c
index b5849b3..bcb4fca 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -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;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index dd9d5e6..2278484 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -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

[Qemu-devel] [PATCH 3/3] block: prohibit migration during transactions

[John Snow]

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.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 32b04b4..231dcf6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -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

Re: [Qemu-devel] [PATCH 3/3] block: prohibit migration during transactions

[Paolo Bonzini]

On 01/10/2015 18:34, John Snow wrote:
> +
> +    error_setg(&blocker, "Block device(s) are in use by a Block Transaction");

s/Block Transaction/transaction command/

But how can migration start during a 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);

Re: [Qemu-devel] [PATCH 3/3] block: prohibit migration during transactions

[John Snow]

On 10/01/2015 02:01 PM, Paolo Bonzini wrote:
> 
> 
> On 01/10/2015 18:34, John Snow wrote:
>> +
>> +    error_setg(&blocker, "Block device(s) are in use by a Block Transaction");
> 
> s/Block Transaction/transaction command/
> 
> But how can migration start during a transaction?
> 

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.

>> +    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);
>