[Qemu-devel] [PATCH 1/7] migrate: add migration blockers

[Qemu-devel] [PATCH 1/7] migrate: add migration blockers

[Anthony Liguori]

This lets different subsystems register an Error that is thrown whenever
migration is attempted.  This works nicely because it gracefully supports
things like hotplug.

Right now, if multiple errors are registered, only one of them is reported.
I expect that for 1.1, we'll extend query-migrate to return all of the reasons
why migration is disabled at any given point in time.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 migration.c |   18 ++++++++++++++++++
 migration.h |   15 +++++++++++++++
 2 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/migration.c b/migration.c
index 41c3c24..6764d3a 100644
--- a/migration.c
+++ b/migration.c
@@ -398,6 +398,18 @@ static MigrationState *migrate_init(Monitor *mon, int detach, int blk, int inc)
     return s;
 }
 
+static GSList *migration_blockers;
+
+void migrate_add_blocker(Error *reason)
+{
+    migration_blockers = g_slist_prepend(migration_blockers, reason);
+}
+
+void migrate_del_blocker(Error *reason)
+{
+    migration_blockers = g_slist_remove(migration_blockers, reason);
+}
+
 int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     MigrationState *s = migrate_get_current();
@@ -417,6 +429,12 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
         return -1;
     }
 
+    if (migration_blockers) {
+        Error *err = migration_blockers->data;
+        qerror_report_err(err);
+        return -1;
+    }
+
     s = migrate_init(mon, detach, blk, inc);
 
     if (strstart(uri, "tcp:", &p)) {
diff --git a/migration.h b/migration.h
index 1b8ee58..0682179 100644
--- a/migration.h
+++ b/migration.h
@@ -17,6 +17,7 @@
 #include "qdict.h"
 #include "qemu-common.h"
 #include "notify.h"
+#include "error.h"
 
 typedef struct MigrationState MigrationState;
 
@@ -89,4 +90,18 @@ int ram_load(QEMUFile *f, void *opaque, int version_id);
 
 extern int incoming_expected;
 
+/**
+ * @migrate_add_blocker - prevent migration from proceeding
+ *
+ * @reason - an error to be returned whenever migration is attempted
+ */
+void migrate_add_blocker(Error *reason);
+
+/**
+ * @migrate_del_blocker - remove a blocking error from migration
+ *
+ * @reason - the error blocking migration
+ */
+void migrate_del_blocker(Error *reason);
+
 #endif
-- 
1.7.4.1

[Qemu-devel] [PATCH 2/7] ivshmem: use migration blockers to prevent live migration in peer mode

[Anthony Liguori]

Now when you try to migrate with ivshmem, you get a proper QMP error:

(qemu) migrate tcp:localhost:1025
Migration is disabled when using feature 'peer mode' in device 'ivshmem'
(qemu)

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/ivshmem.c |   12 +++++++++++-
 qerror.c     |    4 ++++
 qerror.h     |    3 +++
 3 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/hw/ivshmem.c b/hw/ivshmem.c
index 242fbea..a3a0e98 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -18,6 +18,8 @@
 #include "pci.h"
 #include "msix.h"
 #include "kvm.h"
+#include "migration.h"
+#include "qerror.h"
 
 #include <sys/mman.h>
 #include <sys/types.h>
@@ -78,6 +80,8 @@ typedef struct IVShmemState {
     uint32_t features;
     EventfdEntry *eventfd_table;
 
+    Error *migration_blocker;
+
     char * shmobj;
     char * sizearg;
     char * role;
@@ -646,7 +650,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
     }
 
     if (s->role_val == IVSHMEM_PEER) {
-        register_device_unmigratable(&s->dev.qdev, "ivshmem", s);
+        error_set(&s->migration_blocker, QERR_DEVICE_FEATURE_BLOCKS_MIGRATION, "ivshmem", "peer mode");
+        migrate_add_blocker(s->migration_blocker);
     }
 
     pci_conf = s->dev.config;
@@ -741,6 +746,11 @@ static int pci_ivshmem_uninit(PCIDevice *dev)
 {
     IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
 
+    if (s->migration_blocker) {
+        migrate_del_blocker(s->migration_blocker);
+        error_free(s->migration_blocker);
+    }
+
     memory_region_destroy(&s->ivshmem_mmio);
     memory_region_del_subregion(&s->bar, &s->ivshmem);
     memory_region_destroy(&s->ivshmem);
diff --git a/qerror.c b/qerror.c
index 4b48b39..8e30e2d 100644
--- a/qerror.c
+++ b/qerror.c
@@ -73,6 +73,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Device '%(device)' is in use",
     },
     {
+        .error_fmt = QERR_DEVICE_FEATURE_BLOCKS_MIGRATION,
+        .desc      = "Migration is disabled when using feature '%(feature)' in device '%(device)'",
+    },
+    {
         .error_fmt = QERR_DEVICE_LOCKED,
         .desc      = "Device '%(device)' is locked",
     },
diff --git a/qerror.h b/qerror.h
index d4bfcfd..7e2eebf 100644
--- a/qerror.h
+++ b/qerror.h
@@ -72,6 +72,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_DEVICE_IN_USE \
     "{ 'class': 'DeviceInUse', 'data': { 'device': %s } }"
 
+#define QERR_DEVICE_FEATURE_BLOCKS_MIGRATION \
+    "{ 'class': 'DeviceFeatureBlocksMigration', 'data': { 'device': %s, 'feature': %s } }"
+
 #define QERR_DEVICE_LOCKED \
     "{ 'class': 'DeviceLocked', 'data': { 'device': %s } }"
 
-- 
1.7.4.1

[Qemu-devel] [PATCH 3/7] qcow2: add a migration blocker

[Anthony Liguori]

Now when you try to migrate with qcow2, you get:

(qemu) migrate tcp:localhost:1025
Block format 'qcow2' used by device 'ide0-hd0' does not support feature 'live migration'
(qemu)

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 block/qcow2.c |    9 +++++++++
 block/qcow2.h |    2 ++
 qemu-tool.c   |    9 +++++++++
 qerror.c      |    4 ++++
 qerror.h      |    3 +++
 5 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 5c784ee..e4126b7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -29,6 +29,7 @@
 #include "block/qcow2.h"
 #include "qemu-error.h"
 #include "qerror.h"
+#include "migration.h"
 
 /*
   Differences with QCOW:
@@ -277,6 +278,11 @@ static int qcow2_open(BlockDriverState *bs, int flags)
         goto fail;
     }
 
+    error_set(&s->migration_blocker,
+              QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+              "qcow2", bs->device_name, "live migration");
+    migrate_add_blocker(s->migration_blocker);
+
     /* Initialise locks */
     qemu_co_mutex_init(&s->lock);
 
@@ -621,6 +627,9 @@ static void qcow2_close(BlockDriverState *bs)
     BDRVQcowState *s = bs->opaque;
     g_free(s->l1_table);
 
+    migrate_del_blocker(s->migration_blocker);
+    error_free(s->migration_blocker);
+
     qcow2_cache_flush(bs, s->l2_table_cache);
     qcow2_cache_flush(bs, s->refcount_block_cache);
 
diff --git a/block/qcow2.h b/block/qcow2.h
index 531af39..0734b23 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -125,6 +125,8 @@ typedef struct BDRVQcowState {
     int snapshots_size;
     int nb_snapshots;
     QCowSnapshot *snapshots;
+
+    Error *migration_blocker;
 } BDRVQcowState;
 
 /* XXX: use std qcow open function ? */
diff --git a/qemu-tool.c b/qemu-tool.c
index e9f7fe1..5df7279 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -15,6 +15,7 @@
 #include "monitor.h"
 #include "qemu-timer.h"
 #include "qemu-log.h"
+#include "migration.h"
 
 #include <sys/time.h>
 
@@ -92,3 +93,11 @@ int64_t qemu_get_clock_ns(QEMUClock *clock)
 {
     return 0;
 }
+
+void migrate_add_blocker(Error *reason)
+{
+}
+
+void migrate_del_blocker(Error *reason)
+{
+}
diff --git a/qerror.c b/qerror.c
index 8e30e2d..fdf62b9 100644
--- a/qerror.c
+++ b/qerror.c
@@ -49,6 +49,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Device '%(device)' can't go on a %(bad_bus_type) bus",
     },
     {
+        .error_fmt = QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+        .desc      = "Block format '%(format)' used by device '%(name)' does not support feature '%(feature)'",
+    },
+    {
         .error_fmt = QERR_BUS_NOT_FOUND,
         .desc      = "Bus '%(bus)' not found",
     },
diff --git a/qerror.h b/qerror.h
index 7e2eebf..2d3d43b 100644
--- a/qerror.h
+++ b/qerror.h
@@ -54,6 +54,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_BAD_BUS_FOR_DEVICE \
     "{ 'class': 'BadBusForDevice', 'data': { 'device': %s, 'bad_bus_type': %s } }"
 
+#define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \
+    "{ 'class': 'BlockFormatFeatureNotSupported', 'data': { 'format': %s, 'name': %s, 'feature': %s } }"
+
 #define QERR_BUS_NOT_FOUND \
     "{ 'class': 'BusNotFound', 'data': { 'bus': %s } }"
 
-- 
1.7.4.1

[Qemu-devel] [PATCH 4/7] qed: add migration blocker

[Anthony Liguori]

Now when you try to migrate with qed, you get:

(qemu) migrate tcp:localhost:1025
Block format 'qed' used by device 'ide0-hd0' does not support feature 'live migration'
(qemu)

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 block/qed.c |   10 ++++++++++
 block/qed.h |    2 ++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index d032a45..7e22e77 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -16,6 +16,7 @@
 #include "trace.h"
 #include "qed.h"
 #include "qerror.h"
+#include "migration.h"
 
 static void qed_aio_cancel(BlockDriverAIOCB *blockacb)
 {
@@ -504,6 +505,12 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
     s->need_check_timer = qemu_new_timer_ns(vm_clock,
                                             qed_need_check_timer_cb, s);
 
+    error_set(&s->migration_blocker,
+              QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+              "qed", bs->device_name, "live migration");
+    migrate_add_blocker(s->migration_blocker);
+
+
 out:
     if (ret) {
         qed_free_l2_cache(&s->l2_cache);
@@ -516,6 +523,9 @@ static void bdrv_qed_close(BlockDriverState *bs)
 {
     BDRVQEDState *s = bs->opaque;
 
+    migrate_del_blocker(s->migration_blocker);
+    error_free(s->migration_blocker);
+
     qed_cancel_need_check_timer(s);
     qemu_free_timer(s->need_check_timer);
 
diff --git a/block/qed.h b/block/qed.h
index 388fdb3..62cbd3b 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -164,6 +164,8 @@ typedef struct {
 
     /* Periodic flush and clear need check flag */
     QEMUTimer *need_check_timer;
+
+    Error *migration_blocker;
 } BDRVQEDState;
 
 enum {
-- 
1.7.4.1

[Qemu-devel] [PATCH 5/7] block: allow migration to work with image files (v2)

[Anthony Liguori]

Image files have two types of data: immutable data that describes things like
image size, backing files, etc. and mutable data that includes offset and
reference count tables.

Today, image formats aggressively cache mutable data to improve performance.  In
some cases, this happens before a guest even starts.  When dealing with live
migration, since a file is open on two machines, the caching of meta data can
lead to data corruption.

This patch addresses this by introducing a mechanism to invalidate any cached
mutable data a block driver may have which is then used by the live migration
code.

NB, this still requires coherent shared storage.  Addressing migration without
coherent shared storage (i.e. NFS) requires additional work.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
v1 -> v2
 - rebase to latest master
---
 block.c     |   16 ++++++++++++++++
 block.h     |    4 ++++
 block_int.h |    5 +++++
 cpus.c      |    1 +
 migration.c |    3 +++
 5 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 86910b0..d015887 100644
--- a/block.c
+++ b/block.c
@@ -2839,6 +2839,22 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
     }
 }
 
+void bdrv_invalidate_cache(BlockDriverState *bs)
+{
+    if (bs->drv && bs->drv->bdrv_invalidate_cache) {
+        bs->drv->bdrv_invalidate_cache(bs);
+    }
+}
+
+void bdrv_invalidate_cache_all(void)
+{
+    BlockDriverState *bs;
+
+    QTAILQ_FOREACH(bs, &bdrv_states, list) {
+        bdrv_invalidate_cache(bs);
+    }
+}
+
 int bdrv_flush(BlockDriverState *bs)
 {
     Coroutine *co;
diff --git a/block.h b/block.h
index 051a25d..a826059 100644
--- a/block.h
+++ b/block.h
@@ -197,6 +197,10 @@ BlockDriverAIOCB *bdrv_aio_ioctl(BlockDriverState *bs,
         unsigned long int req, void *buf,
         BlockDriverCompletionFunc *cb, void *opaque);
 
+/* Invalidate any cached metadata used by image formats */
+void bdrv_invalidate_cache(BlockDriverState *bs);
+void bdrv_invalidate_cache_all(void);
+
 /* Ensure contents are flushed to disk.  */
 int bdrv_flush(BlockDriverState *bs);
 int coroutine_fn bdrv_co_flush(BlockDriverState *bs);
diff --git a/block_int.h b/block_int.h
index 1ec4921..77c0187 100644
--- a/block_int.h
+++ b/block_int.h
@@ -88,6 +88,11 @@ struct BlockDriver {
         int64_t sector_num, int nb_sectors);
 
     /*
+     * Invalidate any cached meta-data.
+     */
+    void (*bdrv_invalidate_cache)(BlockDriverState *bs);
+
+    /*
      * Flushes all data that was already written to the OS all the way down to
      * the disk (for example raw-posix calls fsync()).
      */
diff --git a/cpus.c b/cpus.c
index 82530c4..ae5ec99 100644
--- a/cpus.c
+++ b/cpus.c
@@ -398,6 +398,7 @@ static void do_vm_stop(RunState state)
         vm_state_notify(0, state);
         qemu_aio_flush();
         bdrv_flush_all();
+        bdrv_invalidate_cache_all();
         monitor_protocol_event(QEVENT_STOP, NULL);
     }
 }
diff --git a/migration.c b/migration.c
index 6764d3a..8280d71 100644
--- a/migration.c
+++ b/migration.c
@@ -89,6 +89,9 @@ void process_incoming_migration(QEMUFile *f)
     qemu_announce_self();
     DPRINTF("successfully loaded vm state\n");
 
+    /* Make sure all file formats flush their mutable metadata */
+    bdrv_invalidate_cache_all();
+
     if (autostart) {
         vm_start();
     } else {
-- 
1.7.4.1

[Qemu-devel] [PATCH 6/7] qcow2: implement bdrv_invalidate_cache

[Anthony Liguori]

We don't reopen the actual file, but instead invoke the close and open routines.
We specifically ignore the backing file since it's contents are read-only and
therefore immutable.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 block/qcow2.c |   19 +++++++++++++++++++
 block/qcow2.h |    2 ++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index e4126b7..b5171e0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -241,6 +241,7 @@ static int qcow2_open(BlockDriverState *bs, int flags)
     s->cluster_data = qemu_blockalign(bs, QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size
                                   + 512);
     s->cluster_cache_offset = -1;
+    s->flags = flags;
 
     ret = qcow2_refcount_init(bs);
     if (ret != 0) {
@@ -641,6 +642,22 @@ static void qcow2_close(BlockDriverState *bs)
     qcow2_refcount_close(bs);
 }
 
+static void qcow2_invalidate_cache(BlockDriverState *bs)
+{
+    BDRVQcowState *s = bs->opaque;
+    int flags = s->flags;
+
+    /*
+     * Backing files are read-only which makes all of their metadata immutable,
+     * that means we don't have to worry about reopening them here.
+     */
+
+    qcow2_close(bs);
+
+    memset(s, 0, sizeof(BDRVQcowState));
+    qcow2_open(bs, flags);
+}
+
 /*
  * Updates the variable length parts of the qcow2 header, i.e. the backing file
  * name and all extensions. qcow2 was not designed to allow such changes, so if
@@ -1278,6 +1295,8 @@ static BlockDriver bdrv_qcow2 = {
 
     .bdrv_change_backing_file   = qcow2_change_backing_file,
 
+    .bdrv_invalidate_cache      = qcow2_invalidate_cache,
+
     .create_options = qcow2_create_options,
     .bdrv_check = qcow2_check,
 };
diff --git a/block/qcow2.h b/block/qcow2.h
index 0734b23..6312cd9 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -127,6 +127,8 @@ typedef struct BDRVQcowState {
     QCowSnapshot *snapshots;
 
     Error *migration_blocker;
+
+    int flags;
 } BDRVQcowState;
 
 /* XXX: use std qcow open function ? */
-- 
1.7.4.1

[Qemu-devel] [PATCH 7/7] qcow2: relax migration blocker

[Anthony Liguori]

Only block migration if we're using encrypted files.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 block/qcow2.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index b5171e0..8071f2c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -279,10 +279,12 @@ static int qcow2_open(BlockDriverState *bs, int flags)
         goto fail;
     }
 
-    error_set(&s->migration_blocker,
-              QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
-              "qcow2", bs->device_name, "live migration");
-    migrate_add_blocker(s->migration_blocker);
+    if (bs->encrypted) {
+        error_set(&s->migration_blocker,
+                  QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+                  "qcow2", bs->device_name, "live migration");
+        migrate_add_blocker(s->migration_blocker);
+    }
 
     /* Initialise locks */
     qemu_co_mutex_init(&s->lock);
@@ -628,8 +630,10 @@ static void qcow2_close(BlockDriverState *bs)
     BDRVQcowState *s = bs->opaque;
     g_free(s->l1_table);
 
-    migrate_del_blocker(s->migration_blocker);
-    error_free(s->migration_blocker);
+    if (s->migration_blocker) {
+        migrate_del_blocker(s->migration_blocker);
+        error_free(s->migration_blocker);
+    }
 
     qcow2_cache_flush(bs, s->l2_table_cache);
     qcow2_cache_flush(bs, s->refcount_block_cache);
-- 
1.7.4.1

[Qemu-devel] [PATCH 5/7] block: allow migration to work with image files (v2)

[Anthony Liguori]

Image files have two types of data: immutable data that describes things like
image size, backing files, etc. and mutable data that includes offset and
reference count tables.

Today, image formats aggressively cache mutable data to improve performance.  In
some cases, this happens before a guest even starts.  When dealing with live
migration, since a file is open on two machines, the caching of meta data can
lead to data corruption.

This patch addresses this by introducing a mechanism to invalidate any cached
mutable data a block driver may have which is then used by the live migration
code.

NB, this still requires coherent shared storage.  Addressing migration without
coherent shared storage (i.e. NFS) requires additional work.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
v1 -> v2
 - rebase to latest master
---
 block.c     |   16 ++++++++++++++++
 block.h     |    4 ++++
 block_int.h |    5 +++++
 cpus.c      |    1 +
 migration.c |    3 +++
 5 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 86910b0..d015887 100644
--- a/block.c
+++ b/block.c
@@ -2839,6 +2839,22 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
     }
 }
 
+void bdrv_invalidate_cache(BlockDriverState *bs)
+{
+    if (bs->drv && bs->drv->bdrv_invalidate_cache) {
+        bs->drv->bdrv_invalidate_cache(bs);
+    }
+}
+
+void bdrv_invalidate_cache_all(void)
+{
+    BlockDriverState *bs;
+
+    QTAILQ_FOREACH(bs, &bdrv_states, list) {
+        bdrv_invalidate_cache(bs);
+    }
+}
+
 int bdrv_flush(BlockDriverState *bs)
 {
     Coroutine *co;
diff --git a/block.h b/block.h
index 051a25d..a826059 100644
--- a/block.h
+++ b/block.h
@@ -197,6 +197,10 @@ BlockDriverAIOCB *bdrv_aio_ioctl(BlockDriverState *bs,
         unsigned long int req, void *buf,
         BlockDriverCompletionFunc *cb, void *opaque);
 
+/* Invalidate any cached metadata used by image formats */
+void bdrv_invalidate_cache(BlockDriverState *bs);
+void bdrv_invalidate_cache_all(void);
+
 /* Ensure contents are flushed to disk.  */
 int bdrv_flush(BlockDriverState *bs);
 int coroutine_fn bdrv_co_flush(BlockDriverState *bs);
diff --git a/block_int.h b/block_int.h
index 1ec4921..77c0187 100644
--- a/block_int.h
+++ b/block_int.h
@@ -88,6 +88,11 @@ struct BlockDriver {
         int64_t sector_num, int nb_sectors);
 
     /*
+     * Invalidate any cached meta-data.
+     */
+    void (*bdrv_invalidate_cache)(BlockDriverState *bs);
+
+    /*
      * Flushes all data that was already written to the OS all the way down to
      * the disk (for example raw-posix calls fsync()).
      */
diff --git a/cpus.c b/cpus.c
index 82530c4..ae5ec99 100644
--- a/cpus.c
+++ b/cpus.c
@@ -398,6 +398,7 @@ static void do_vm_stop(RunState state)
         vm_state_notify(0, state);
         qemu_aio_flush();
         bdrv_flush_all();
+        bdrv_invalidate_cache_all();
         monitor_protocol_event(QEVENT_STOP, NULL);
     }
 }
diff --git a/migration.c b/migration.c
index 6764d3a..8280d71 100644
--- a/migration.c
+++ b/migration.c
@@ -89,6 +89,9 @@ void process_incoming_migration(QEMUFile *f)
     qemu_announce_self();
     DPRINTF("successfully loaded vm state\n");
 
+    /* Make sure all file formats flush their mutable metadata */
+    bdrv_invalidate_cache_all();
+
     if (autostart) {
         vm_start();
     } else {
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 7/7] qcow2: relax migration blocker

[Kevin Wolf]

Am 12.11.2011 16:57, schrieb Anthony Liguori:
> Only block migration if we're using encrypted files.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Come on, it would have been easy enough to do it properly in patch 6 and
keep the key, so that encryption just works. :-)

Kevin

Re: [Qemu-devel] [PATCH 1/7] migrate: add migration blockers

[Kevin Wolf]

Am 12.11.2011 16:56, schrieb Anthony Liguori:
> This lets different subsystems register an Error that is thrown whenever
> migration is attempted.  This works nicely because it gracefully supports
> things like hotplug.
> 
> Right now, if multiple errors are registered, only one of them is reported.
> I expect that for 1.1, we'll extend query-migrate to return all of the reasons
> why migration is disabled at any given point in time.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  migration.c |   18 ++++++++++++++++++
>  migration.h |   15 +++++++++++++++
>  2 files changed, 33 insertions(+), 0 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index 41c3c24..6764d3a 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -398,6 +398,18 @@ static MigrationState *migrate_init(Monitor *mon, int detach, int blk, int inc)
>      return s;
>  }
>  
> +static GSList *migration_blockers;

Any reason not to use qemu-queue.h? It's used everywhere else and being
consistent is usually a good thing. If you want to switch to the glib
functions as the standard for whatever reason, shouldn't we convert
everything at once?

Kevin

Re: [Qemu-devel] [PATCH 1/7] migrate: add migration blockers

[Kevin Wolf]

Am 12.11.2011 16:56, schrieb Anthony Liguori:
> This lets different subsystems register an Error that is thrown whenever
> migration is attempted.  This works nicely because it gracefully supports
> things like hotplug.
> 
> Right now, if multiple errors are registered, only one of them is reported.
> I expect that for 1.1, we'll extend query-migrate to return all of the reasons
> why migration is disabled at any given point in time.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  migration.c |   18 ++++++++++++++++++
>  migration.h |   15 +++++++++++++++
>  2 files changed, 33 insertions(+), 0 deletions(-)

There doesn't seem to be a patch 0/n, so it goes here...

The series looks good in general. I commented on two patches, and there
are a few more image formats that must block migration (basically
everything that is writable, I would be surprised if there was one that
didn't cache anything).

Kevin

Re: [Qemu-devel] [PATCH 1/7] migrate: add migration blockers

[Juan Quintela]

Anthony Liguori <aliguori@us.ibm.com> wrote:
> This lets different subsystems register an Error that is thrown whenever
> migration is attempted.  This works nicely because it gracefully supports
> things like hotplug.
>
> Right now, if multiple errors are registered, only one of them is reported.
> I expect that for 1.1, we'll extend query-migrate to return all of the reasons
> why migration is disabled at any given point in time.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  migration.c |   18 ++++++++++++++++++
>  migration.h |   15 +++++++++++++++
>  2 files changed, 33 insertions(+), 0 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index 41c3c24..6764d3a 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -398,6 +398,18 @@ static MigrationState *migrate_init(Monitor *mon, int detach, int blk, int inc)
>      return s;
>  }
>  
> +static GSList *migration_blockers;
> +

As said by Kevin, using another list implementation makes no sense on my
book.

Rest of idea ok.

Re: [Qemu-devel] [PATCH 2/7] ivshmem: use migration blockers to prevent live migration in peer mode

[Juan Quintela]

Anthony Liguori <aliguori@us.ibm.com> wrote:
> Now when you try to migrate with ivshmem, you get a proper QMP error:
>
> (qemu) migrate tcp:localhost:1025
> Migration is disabled when using feature 'peer mode' in device 'ivshmem'
> (qemu)

This was the only user of register_device_unmigratable(), just to remove
function if we continue this path.

Re: [Qemu-devel] [PATCH 5/7] block: allow migration to work with image files (v2)

[Juan Quintela]

> diff --git a/cpus.c b/cpus.c
> index 82530c4..ae5ec99 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -398,6 +398,7 @@ static void do_vm_stop(RunState state)
>          vm_state_notify(0, state);
>          qemu_aio_flush();
>          bdrv_flush_all();
> +        bdrv_invalidate_cache_all();
>          monitor_protocol_event(QEVENT_STOP, NULL);
>      }

This is too much. Reopening all qcow2 images each time that we stop the
vm looks excesive, no?

Later, Juan.

Re: [Qemu-devel] [PATCH 2/7] ivshmem: use migration blockers to prevent live migration in peer mode

[Anthony Liguori]

On 11/14/2011 07:05 AM, Juan Quintela wrote:
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>> Now when you try to migrate with ivshmem, you get a proper QMP error:
>>
>> (qemu) migrate tcp:localhost:1025
>> Migration is disabled when using feature 'peer mode' in device 'ivshmem'
>> (qemu)
>
> This was the only user of register_device_unmigratable(), just to remove
> function if we continue this path.

There are a couple more in usb also but I can convert them too.

Regards,

Anthony Liguori

>

Re: [Qemu-devel] [PATCH 5/7] block: allow migration to work with image files (v2)

[Anthony Liguori]

On 11/14/2011 07:11 AM, Juan Quintela wrote:
>
>> diff --git a/cpus.c b/cpus.c
>> index 82530c4..ae5ec99 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -398,6 +398,7 @@ static void do_vm_stop(RunState state)
>>           vm_state_notify(0, state);
>>           qemu_aio_flush();
>>           bdrv_flush_all();
>> +        bdrv_invalidate_cache_all();
>>           monitor_protocol_event(QEVENT_STOP, NULL);
>>       }
>
> This is too much. Reopening all qcow2 images each time that we stop the
> vm looks excesive, no?

This general code came in via:

http://mid.gmane.org/cover.1290613959.git.mst@redhat.com

That series made migration stable after issuing a stop operation.  I believe the 
justification was for debugging purposes or something like that.

At any rate, invalidating the cache is part of what's required to make things 
stable.  If you look at something like cache=unsafe, the only way the metadata 
will get flushed if via a bdrv_close since bdrv_flush is a nop.

So this is needed as long as we care about supporting this use-case.

Regards,

Anthony Liguori

>
> Later, Juan.
>

Re: [Qemu-devel] [PATCH 1/7] migrate: add migration blockers

[Anthony Liguori]

On 11/14/2011 03:08 AM, Kevin Wolf wrote:
> Am 12.11.2011 16:56, schrieb Anthony Liguori:
>> This lets different subsystems register an Error that is thrown whenever
>> migration is attempted.  This works nicely because it gracefully supports
>> things like hotplug.
>>
>> Right now, if multiple errors are registered, only one of them is reported.
>> I expect that for 1.1, we'll extend query-migrate to return all of the reasons
>> why migration is disabled at any given point in time.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   migration.c |   18 ++++++++++++++++++
>>   migration.h |   15 +++++++++++++++
>>   2 files changed, 33 insertions(+), 0 deletions(-)
>>
>> diff --git a/migration.c b/migration.c
>> index 41c3c24..6764d3a 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -398,6 +398,18 @@ static MigrationState *migrate_init(Monitor *mon, int detach, int blk, int inc)
>>       return s;
>>   }
>>
>> +static GSList *migration_blockers;
>
> Any reason not to use qemu-queue.h? It's used everywhere else and being
> consistent is usually a good thing. If you want to switch to the glib
> functions as the standard for whatever reason, shouldn't we convert
> everything at once?

They're fundamentally different list implementations.  GSList is a container 
while qemu-queues are embedded lists.  In this case, it's impossible to use 
qemu-queue without creating another data structure which would make things more 
complex than they need to be.

They aren't exact replacements for each other so I think they can co-exist and 
be used where they make sense respectively.

Regards,

Anthony Liguori

>
> Kevin
>

Re: [Qemu-devel] [PATCH 5/7] block: allow migration to work with image files (v2)

[Juan Quintela]

Anthony Liguori <aliguori@us.ibm.com> wrote:
> On 11/14/2011 07:11 AM, Juan Quintela wrote:
>>
>>> diff --git a/cpus.c b/cpus.c
>>> index 82530c4..ae5ec99 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -398,6 +398,7 @@ static void do_vm_stop(RunState state)
>>>           vm_state_notify(0, state);
>>>           qemu_aio_flush();
>>>           bdrv_flush_all();
>>> +        bdrv_invalidate_cache_all();
>>>           monitor_protocol_event(QEVENT_STOP, NULL);
>>>       }
>>
>> This is too much. Reopening all qcow2 images each time that we stop the
>> vm looks excesive, no?
>
> This general code came in via:
>
> http://mid.gmane.org/cover.1290613959.git.mst@redhat.com
>
> That series made migration stable after issuing a stop operation.  I
> believe the justification was for debugging purposes or something like
> that.
>
> At any rate, invalidating the cache is part of what's required to make
> things stable.  If you look at something like cache=unsafe, the only
> way the metadata will get flushed if via a bdrv_close since bdrv_flush
> is a nop.
>
> So this is needed as long as we care about supporting this use-case.

Then we need a "proper" qcow2 invalidate call.  Doing in qemu toplevel:

(qemu)stop

And now all your qcow2 block devices are closed, or perhaps failing to
re-open() looks too much to me (TM).

Kevin?

Later, Juan.

Re: [Qemu-devel] [PATCH 5/7] block: allow migration to work with image files (v2)

[Anthony Liguori]

On 11/14/2011 01:46 PM, Juan Quintela wrote:
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>> On 11/14/2011 07:11 AM, Juan Quintela wrote:
>>>
>>>> diff --git a/cpus.c b/cpus.c
>>>> index 82530c4..ae5ec99 100644
>>>> --- a/cpus.c
>>>> +++ b/cpus.c
>>>> @@ -398,6 +398,7 @@ static void do_vm_stop(RunState state)
>>>>            vm_state_notify(0, state);
>>>>            qemu_aio_flush();
>>>>            bdrv_flush_all();
>>>> +        bdrv_invalidate_cache_all();
>>>>            monitor_protocol_event(QEVENT_STOP, NULL);
>>>>        }
>>>
>>> This is too much. Reopening all qcow2 images each time that we stop the
>>> vm looks excesive, no?
>>
>> This general code came in via:
>>
>> http://mid.gmane.org/cover.1290613959.git.mst@redhat.com
>>
>> That series made migration stable after issuing a stop operation.  I
>> believe the justification was for debugging purposes or something like
>> that.
>>
>> At any rate, invalidating the cache is part of what's required to make
>> things stable.  If you look at something like cache=unsafe, the only
>> way the metadata will get flushed if via a bdrv_close since bdrv_flush
>> is a nop.
>>
>> So this is needed as long as we care about supporting this use-case.
>
> Then we need a "proper" qcow2 invalidate call.  Doing in qemu toplevel:
>
> (qemu)stop
>
> And now all your qcow2 block devices are closed, or perhaps failing to
> re-open() looks too much to me (TM).
>
> Kevin?

Look closely at the patch.  It doesn't actually close()/open() anything.

It just invokes the bdrv_close() routine which calls the free functions on the 
l1/l2 caching functions.  bdrv_open() doesn't actually open anything (it assumes 
the file is already open.  It just reads the header and metadata over again.

For something that's basically a hack, it turned out to work very cleanly :-)

Regards,

Anthony Liguori

>
> Later, Juan.
>
>

Re: [Qemu-devel] [PATCH 2/7] ivshmem: use migration blockers to prevent live migration in peer mode

[Juan Quintela]

Anthony Liguori <aliguori@us.ibm.com> wrote:
> On 11/14/2011 07:05 AM, Juan Quintela wrote:
>> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>>> Now when you try to migrate with ivshmem, you get a proper QMP error:
>>>
>>> (qemu) migrate tcp:localhost:1025
>>> Migration is disabled when using feature 'peer mode' in device 'ivshmem'
>>> (qemu)
>>
>> This was the only user of register_device_unmigratable(), just to remove
>> function if we continue this path.
>
> There are a couple more in usb also but I can convert them too.

usb uses "yet another" way to make devices unmigratable.

static const VMStateDescription vmstate_usb_host = {
    .name = "usb-host",
    .unmigratable = 1,
};

So, we have "already" two ways to make a device unmigratable:
- calling register_device_unmigratable() (ivhs)
- definining in vmstate .unmigratable = 1
- and now your new way.

My point was that only one (or even two) should be enough.

Why two? because the .unmigratable way is very useful for devices that
haven't been made "migratable", but that are converted to qdev.  Not
jthat this couldn't be "fixed" on registration with a call to
migrate_add_blocker.

Later, Juan.

Re: [Qemu-devel] [PATCH 2/7] ivshmem: use migration blockers to prevent live migration in peer mode

[Anthony Liguori]

On 11/14/2011 01:52 PM, Juan Quintela wrote:
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>> On 11/14/2011 07:05 AM, Juan Quintela wrote:
>>> Anthony Liguori<aliguori@us.ibm.com>   wrote:
>>>> Now when you try to migrate with ivshmem, you get a proper QMP error:
>>>>
>>>> (qemu) migrate tcp:localhost:1025
>>>> Migration is disabled when using feature 'peer mode' in device 'ivshmem'
>>>> (qemu)
>>>
>>> This was the only user of register_device_unmigratable(), just to remove
>>> function if we continue this path.
>>
>> There are a couple more in usb also but I can convert them too.
>
> usb uses "yet another" way to make devices unmigratable.
>
> static const VMStateDescription vmstate_usb_host = {
>      .name = "usb-host",
>      .unmigratable = 1,
> };
>
> So, we have "already" two ways to make a device unmigratable:
> - calling register_device_unmigratable() (ivhs)
> - definining in vmstate .unmigratable = 1
> - and now your new way.
>
> My point was that only one (or even two) should be enough.

Ah, yes, I see now.  I'll remove that function.

Regards,

Anthony Liguori

>
> Why two? because the .unmigratable way is very useful for devices that
> haven't been made "migratable", but that are converted to qdev.  Not
> jthat this couldn't be "fixed" on registration with a call to
> migrate_add_blocker.
>
> Later, Juan.
>

Re: [Qemu-devel] [PATCH 5/7] block: allow migration to work with image files (v2)

[Kevin Wolf]

Am 14.11.2011 20:49, schrieb Anthony Liguori:
> On 11/14/2011 01:46 PM, Juan Quintela wrote:
>> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>>> On 11/14/2011 07:11 AM, Juan Quintela wrote:
>>>>
>>>>> diff --git a/cpus.c b/cpus.c
>>>>> index 82530c4..ae5ec99 100644
>>>>> --- a/cpus.c
>>>>> +++ b/cpus.c
>>>>> @@ -398,6 +398,7 @@ static void do_vm_stop(RunState state)
>>>>>            vm_state_notify(0, state);
>>>>>            qemu_aio_flush();
>>>>>            bdrv_flush_all();
>>>>> +        bdrv_invalidate_cache_all();
>>>>>            monitor_protocol_event(QEVENT_STOP, NULL);
>>>>>        }
>>>>
>>>> This is too much. Reopening all qcow2 images each time that we stop the
>>>> vm looks excesive, no?
>>>
>>> This general code came in via:
>>>
>>> http://mid.gmane.org/cover.1290613959.git.mst@redhat.com
>>>
>>> That series made migration stable after issuing a stop operation.  I
>>> believe the justification was for debugging purposes or something like
>>> that.
>>>
>>> At any rate, invalidating the cache is part of what's required to make
>>> things stable.  If you look at something like cache=unsafe, the only
>>> way the metadata will get flushed if via a bdrv_close since bdrv_flush
>>> is a nop.
>>>
>>> So this is needed as long as we care about supporting this use-case.
>>
>> Then we need a "proper" qcow2 invalidate call.  Doing in qemu toplevel:
>>
>> (qemu)stop
>>
>> And now all your qcow2 block devices are closed, or perhaps failing to
>> re-open() looks too much to me (TM).
>>
>> Kevin?
> 
> Look closely at the patch.  It doesn't actually close()/open() anything.
> 
> It just invokes the bdrv_close() routine which calls the free functions on the 
> l1/l2 caching functions.  bdrv_open() doesn't actually open anything (it assumes 
> the file is already open.  It just reads the header and metadata over again.
> 
> For something that's basically a hack, it turned out to work very cleanly :-)

But why do we need to do it on stop?

I don't think it makes even sense logically: bdrv_invalidate_cache()
means "throw all your caches away and refetch everything from disk".
What do we gain from doing this on stop? To some degree I could
understand if you did it on cont, so that you can modify an image on the
host while the VM is stopped (though I would still consider it criminal
:-)).

Kevin

Re: [Qemu-devel] [PATCH 5/7] block: allow migration to work with image files (v2)

[Anthony Liguori]

On 11/14/2011 02:11 PM, Kevin Wolf wrote:
> Am 14.11.2011 20:49, schrieb Anthony Liguori:
>> On 11/14/2011 01:46 PM, Juan Quintela wrote:
>>> Anthony Liguori<aliguori@us.ibm.com>   wrote:
>>>> On 11/14/2011 07:11 AM, Juan Quintela wrote:
>>>>>
>>>>>> diff --git a/cpus.c b/cpus.c
>>>>>> index 82530c4..ae5ec99 100644
>>>>>> --- a/cpus.c
>>>>>> +++ b/cpus.c
>>>>>> @@ -398,6 +398,7 @@ static void do_vm_stop(RunState state)
>>>>>>             vm_state_notify(0, state);
>>>>>>             qemu_aio_flush();
>>>>>>             bdrv_flush_all();
>>>>>> +        bdrv_invalidate_cache_all();
>>>>>>             monitor_protocol_event(QEVENT_STOP, NULL);
>>>>>>         }
>>>>>
>>>>> This is too much. Reopening all qcow2 images each time that we stop the
>>>>> vm looks excesive, no?
>>>>
>>>> This general code came in via:
>>>>
>>>> http://mid.gmane.org/cover.1290613959.git.mst@redhat.com
>>>>
>>>> That series made migration stable after issuing a stop operation.  I
>>>> believe the justification was for debugging purposes or something like
>>>> that.
>>>>
>>>> At any rate, invalidating the cache is part of what's required to make
>>>> things stable.  If you look at something like cache=unsafe, the only
>>>> way the metadata will get flushed if via a bdrv_close since bdrv_flush
>>>> is a nop.
>>>>
>>>> So this is needed as long as we care about supporting this use-case.
>>>
>>> Then we need a "proper" qcow2 invalidate call.  Doing in qemu toplevel:
>>>
>>> (qemu)stop
>>>
>>> And now all your qcow2 block devices are closed, or perhaps failing to
>>> re-open() looks too much to me (TM).
>>>
>>> Kevin?
>>
>> Look closely at the patch.  It doesn't actually close()/open() anything.
>>
>> It just invokes the bdrv_close() routine which calls the free functions on the
>> l1/l2 caching functions.  bdrv_open() doesn't actually open anything (it assumes
>> the file is already open.  It just reads the header and metadata over again.
>>
>> For something that's basically a hack, it turned out to work very cleanly :-)
>
> But why do we need to do it on stop?
>
> I don't think it makes even sense logically: bdrv_invalidate_cache()
> means "throw all your caches away and refetch everything from disk".
> What do we gain from doing this on stop? To some degree I could
> understand if you did it on cont, so that you can modify an image on the
> host while the VM is stopped (though I would still consider it criminal
> :-)).

Michael basically was trying to avoid having a VM's state change after you 
stopped the guest.

With something like cache=unsafe that periodically flushes based on a timer (I 
think), you want to make sure that that doesn't happen after stop occurs.

Regards,

Anthony Liguori

>
> Kevin
>
>

Re: [Qemu-devel] [PATCH 5/7] block: allow migration to work with image files (v2)

[Juan Quintela]

Kevin Wolf <kwolf@redhat.com> wrote:
> Am 14.11.2011 20:49, schrieb Anthony Liguori:
>> On 11/14/2011 01:46 PM, Juan Quintela wrote:
>>> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>>>> On 11/14/2011 07:11 AM, Juan Quintela wrote:
>>>>>
>>>>>> diff --git a/cpus.c b/cpus.c
>>>>>> index 82530c4..ae5ec99 100644
>>>>>> --- a/cpus.c
>>>>>> +++ b/cpus.c
>>>>>> @@ -398,6 +398,7 @@ static void do_vm_stop(RunState state)
>>>>>>            vm_state_notify(0, state);
>>>>>>            qemu_aio_flush();
>>>>>>            bdrv_flush_all();
>>>>>> +        bdrv_invalidate_cache_all();
>>>>>>            monitor_protocol_event(QEVENT_STOP, NULL);
>>>>>>        }
>>>>>
>>>>> This is too much. Reopening all qcow2 images each time that we stop the
>>>>> vm looks excesive, no?
>>>>
>>>> This general code came in via:
>>>>
>>>> http://mid.gmane.org/cover.1290613959.git.mst@redhat.com
>>>>
>>>> That series made migration stable after issuing a stop operation.  I
>>>> believe the justification was for debugging purposes or something like
>>>> that.
>>>>
>>>> At any rate, invalidating the cache is part of what's required to make
>>>> things stable.  If you look at something like cache=unsafe, the only
>>>> way the metadata will get flushed if via a bdrv_close since bdrv_flush
>>>> is a nop.
>>>>
>>>> So this is needed as long as we care about supporting this use-case.
>>>
>>> Then we need a "proper" qcow2 invalidate call.  Doing in qemu toplevel:
>>>
>>> (qemu)stop
>>>
>>> And now all your qcow2 block devices are closed, or perhaps failing to
>>> re-open() looks too much to me (TM).
>>>
>>> Kevin?
>> 
>> Look closely at the patch.  It doesn't actually close()/open() anything.

Sorry, someday I will remember the difference between bdrv_open() and
bdrv_file_open().

>> It just invokes the bdrv_close() routine which calls the free functions on the 
>> l1/l2 caching functions.  bdrv_open() doesn't actually open anything (it assumes 
>> the file is already open.  It just reads the header and metadata over again.
>> 
>> For something that's basically a hack, it turned out to work very cleanly :-)
>
> But why do we need to do it on stop?
>
> I don't think it makes even sense logically: bdrv_invalidate_cache()
> means "throw all your caches away and refetch everything from disk".
> What do we gain from doing this on stop? To some degree I could
> understand if you did it on cont, so that you can modify an image on the
> host while the VM is stopped (though I would still consider it criminal
> :-)).

Fully agree.  When I answered, I was thinking that I "could" want it on
"cont", just to be able to do evil things.  But I thought it was
"criminal" just to write the idea O:-)

Later, Juan.

Re: [Qemu-devel] [PATCH 5/7] block: allow migration to work with image files (v2)

[Juan Quintela]

Anthony Liguori <anthony@codemonkey.ws> wrote:

>
> Michael basically was trying to avoid having a VM's state change after
> you stopped the guest.
>
> With something like cache=unsafe that periodically flushes based on a
> timer (I think), you want to make sure that that doesn't happen after
> stop occurs.

Even then, we want to have "qcow2_flush_dirty_buffer_to_disk()" or
whatever is going to be called the method.  Doing a full "drop all
cache" and "reread" it is using a cannon to fight flies IMHO.
Especially because there are "lots" of uses of stop, and only a minority
of them want this flushed to disk (if ever).

My recount of mst problems were with networking (vhost) than something
touch the guest image while VM is stopped.  This is not the case here,
as we are not doing anything with the guest memory, no?

Later, Juan.

Re: [Qemu-devel] [PATCH 5/7] block: allow migration to work with image files (v2)

[Kevin Wolf]

Am 14.11.2011 21:12, schrieb Anthony Liguori:
> On 11/14/2011 02:11 PM, Kevin Wolf wrote:
>> Am 14.11.2011 20:49, schrieb Anthony Liguori:
>>> On 11/14/2011 01:46 PM, Juan Quintela wrote:
>>>> Anthony Liguori<aliguori@us.ibm.com>   wrote:
>>>>> On 11/14/2011 07:11 AM, Juan Quintela wrote:
>>>>>>
>>>>>>> diff --git a/cpus.c b/cpus.c
>>>>>>> index 82530c4..ae5ec99 100644
>>>>>>> --- a/cpus.c
>>>>>>> +++ b/cpus.c
>>>>>>> @@ -398,6 +398,7 @@ static void do_vm_stop(RunState state)
>>>>>>>             vm_state_notify(0, state);
>>>>>>>             qemu_aio_flush();
>>>>>>>             bdrv_flush_all();
>>>>>>> +        bdrv_invalidate_cache_all();
>>>>>>>             monitor_protocol_event(QEVENT_STOP, NULL);
>>>>>>>         }
>>>>>>
>>>>>> This is too much. Reopening all qcow2 images each time that we stop the
>>>>>> vm looks excesive, no?
>>>>>
>>>>> This general code came in via:
>>>>>
>>>>> http://mid.gmane.org/cover.1290613959.git.mst@redhat.com
>>>>>
>>>>> That series made migration stable after issuing a stop operation.  I
>>>>> believe the justification was for debugging purposes or something like
>>>>> that.
>>>>>
>>>>> At any rate, invalidating the cache is part of what's required to make
>>>>> things stable.  If you look at something like cache=unsafe, the only
>>>>> way the metadata will get flushed if via a bdrv_close since bdrv_flush
>>>>> is a nop.
>>>>>
>>>>> So this is needed as long as we care about supporting this use-case.
>>>>
>>>> Then we need a "proper" qcow2 invalidate call.  Doing in qemu toplevel:
>>>>
>>>> (qemu)stop
>>>>
>>>> And now all your qcow2 block devices are closed, or perhaps failing to
>>>> re-open() looks too much to me (TM).
>>>>
>>>> Kevin?
>>>
>>> Look closely at the patch.  It doesn't actually close()/open() anything.
>>>
>>> It just invokes the bdrv_close() routine which calls the free functions on the
>>> l1/l2 caching functions.  bdrv_open() doesn't actually open anything (it assumes
>>> the file is already open.  It just reads the header and metadata over again.
>>>
>>> For something that's basically a hack, it turned out to work very cleanly :-)
>>
>> But why do we need to do it on stop?
>>
>> I don't think it makes even sense logically: bdrv_invalidate_cache()
>> means "throw all your caches away and refetch everything from disk".
>> What do we gain from doing this on stop? To some degree I could
>> understand if you did it on cont, so that you can modify an image on the
>> host while the VM is stopped (though I would still consider it criminal
>> :-)).
> 
> Michael basically was trying to avoid having a VM's state change after you 
> stopped the guest.
> 
> With something like cache=unsafe that periodically flushes based on a timer (I 
> think), you want to make sure that that doesn't happen after stop occurs.

This is a good point, but neither does cache=unsafe use a timer nor can
I see how invalidating the cache would avoid such behaviour. And
throwing away any unwritten changes doesn't really make it better.

Kevin

Re: [Qemu-devel] [PATCH 5/7] block: allow migration to work with image files (v2)

[Anthony Liguori]

On 11/14/2011 02:36 PM, Kevin Wolf wrote:
> Am 14.11.2011 21:12, schrieb Anthony Liguori:
>>> I don't think it makes even sense logically: bdrv_invalidate_cache()
>>> means "throw all your caches away and refetch everything from disk".
>>> What do we gain from doing this on stop? To some degree I could
>>> understand if you did it on cont, so that you can modify an image on the
>>> host while the VM is stopped (though I would still consider it criminal
>>> :-)).
>>
>> Michael basically was trying to avoid having a VM's state change after you
>> stopped the guest.
>>
>> With something like cache=unsafe that periodically flushes based on a timer (I
>> think), you want to make sure that that doesn't happen after stop occurs.
>
> This is a good point, but neither does cache=unsafe use a timer nor can
> I see how invalidating the cache would avoid such behaviour. And
> throwing away any unwritten changes doesn't really make it better.

I don't think there's any real harm to removing it so I'll remove it in the next 
rev.

Regards,

Anthony Liguori

>
> Kevin
>