[Qemu-devel] [PATCH 0/3] dataplane: virtio-blk live migration with x-data-plane=on

[Qemu-devel] [PATCH 0/3] dataplane: virtio-blk live migration with x-data-plane=on

[Stefan Hajnoczi]

These patches add live migration support to -device virtio-blk-pci,x-data-plane=on.

Patch 1 has already been posted and merged into the block tree.  I have
included it for convenience.

Patches 2 & 3 implement a switch from dataplane mode back to regular virtio-blk
mode when migration starts.  This way live migration works.

If migration is cancelled or the guest accesses the virtio-blk device after
completion, dataplane starts again.

Since this approach is so small, it's more palatable for QEMU 1.6 than trying
to make vring.c log dirty memory.  It makes dataplane usable in situations
where live migration is a requirement.

Stefan Hajnoczi (3):
  dataplane: sync virtio.c and vring.c virtqueue state
  migration: notify migration state before starting thread
  dataplane: enable virtio-blk x-data-plane=on live migration

 hw/block/dataplane/virtio-blk.c     | 19 +++++++++----------
 hw/block/virtio-blk.c               | 32 ++++++++++++++++++++++++++++++++
 hw/virtio/dataplane/vring.c         |  8 +++++---
 include/hw/virtio/dataplane/vring.h |  2 +-
 include/hw/virtio/virtio-blk.h      |  1 +
 migration.c                         |  4 +++-
 6 files changed, 51 insertions(+), 15 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/3] dataplane: sync virtio.c and vring.c virtqueue state

[Stefan Hajnoczi]

Load the virtio.c state into vring.c when we start dataplane mode and
vice versa when stopping dataplane mode.  This patch makes it possible
to start and stop dataplane any time while the guest is running.

This is very useful since it will allow us to go back to QEMU main loop
for bdrv_drain_all() and live migration.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/dataplane/virtio-blk.c     | 2 +-
 hw/virtio/dataplane/vring.c         | 8 +++++---
 include/hw/virtio/dataplane/vring.h | 2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 0356665..2faed43 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -537,7 +537,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
     /* Clean up guest notifier (irq) */
     k->set_guest_notifiers(qbus->parent, 1, false);
 
-    vring_teardown(&s->vring);
+    vring_teardown(&s->vring, s->vdev, 0);
     s->started = false;
     s->stopping = false;
 }
diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index e0d6e83..82cc151 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -39,8 +39,8 @@ bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
 
     vring_init(&vring->vr, virtio_queue_get_num(vdev, n), vring_ptr, 4096);
 
-    vring->last_avail_idx = 0;
-    vring->last_used_idx = 0;
+    vring->last_avail_idx = virtio_queue_get_last_avail_idx(vdev, n);
+    vring->last_used_idx = vring->vr.used->idx;
     vring->signalled_used = 0;
     vring->signalled_used_valid = false;
 
@@ -49,8 +49,10 @@ bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
     return true;
 }
 
-void vring_teardown(Vring *vring)
+void vring_teardown(Vring *vring, VirtIODevice *vdev, int n)
 {
+    virtio_queue_set_last_avail_idx(vdev, n, vring->last_avail_idx);
+
     hostmem_finalize(&vring->hostmem);
 }
 
diff --git a/include/hw/virtio/dataplane/vring.h b/include/hw/virtio/dataplane/vring.h
index 9380cb5..c0b69ff 100644
--- a/include/hw/virtio/dataplane/vring.h
+++ b/include/hw/virtio/dataplane/vring.h
@@ -50,7 +50,7 @@ static inline void vring_set_broken(Vring *vring)
 }
 
 bool vring_setup(Vring *vring, VirtIODevice *vdev, int n);
-void vring_teardown(Vring *vring);
+void vring_teardown(Vring *vring, VirtIODevice *vdev, int n);
 void vring_disable_notification(VirtIODevice *vdev, Vring *vring);
 bool vring_enable_notification(VirtIODevice *vdev, Vring *vring);
 bool vring_should_notify(VirtIODevice *vdev, Vring *vring);
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/3] migration: notify migration state before starting thread

[Stefan Hajnoczi]

The migration thread runs outside the QEMU global mutex when possible.
Therefore we must notify migration state change *before* starting the
migration thread.

This allows registered listeners to act before live migration iterations
begin.  Therefore they can get into a state that allows for live
migration.  When the migration thread starts everything will be ready.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 migration.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/migration.c b/migration.c
index 9f5a423..b4daf13 100644
--- a/migration.c
+++ b/migration.c
@@ -625,7 +625,9 @@ void migrate_fd_connect(MigrationState *s)
     qemu_file_set_rate_limit(s->file,
                              s->bandwidth_limit / XFER_LIMIT_RATIO);
 
+    /* Notify before starting migration thread */
+    notifier_list_notify(&migration_state_notifiers, s);
+
     qemu_thread_create(&s->thread, migration_thread, s,
                        QEMU_THREAD_JOINABLE);
-    notifier_list_notify(&migration_state_notifiers, s);
 }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 2/3] migration: notify migration state before starting thread

[Paolo Bonzini]

Il 17/07/2013 11:35, Stefan Hajnoczi ha scritto:
> The migration thread runs outside the QEMU global mutex when possible.
> Therefore we must notify migration state change *before* starting the
> migration thread.
> 
> This allows registered listeners to act before live migration iterations
> begin.  Therefore they can get into a state that allows for live
> migration.  When the migration thread starts everything will be ready.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  migration.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/migration.c b/migration.c
> index 9f5a423..b4daf13 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -625,7 +625,9 @@ void migrate_fd_connect(MigrationState *s)
>      qemu_file_set_rate_limit(s->file,
>                               s->bandwidth_limit / XFER_LIMIT_RATIO);
>  
> +    /* Notify before starting migration thread */
> +    notifier_list_notify(&migration_state_notifiers, s);
> +
>      qemu_thread_create(&s->thread, migration_thread, s,
>                         QEMU_THREAD_JOINABLE);
> -    notifier_list_notify(&migration_state_notifiers, s);
>  }
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

[Qemu-devel] [PATCH 3/3] dataplane: enable virtio-blk x-data-plane=on live migration

[Stefan Hajnoczi]

Although the dataplane thread does not cooperate with dirty memory
logging yet it's fairly easy to temporarily disable dataplane during
live migration.  This way virtio-blk can live migrate when
x-data-plane=on.

The dataplane thread will restart after migration is cancelled or if the
guest resuming virtio-blk operation after migration completes.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 17 ++++++++---------
 hw/block/virtio-blk.c           | 32 ++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio-blk.h  |  1 +
 3 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 2faed43..411becc 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -18,7 +18,6 @@
 #include "qemu/error-report.h"
 #include "hw/virtio/dataplane/vring.h"
 #include "ioq.h"
-#include "migration/migration.h"
 #include "block/block.h"
 #include "hw/virtio/virtio-blk.h"
 #include "virtio-blk.h"
@@ -69,8 +68,6 @@ struct VirtIOBlockDataPlane {
                                              queue */
 
     unsigned int num_reqs;
-
-    Error *migration_blocker;
 };
 
 /* Raise an interrupt to signal guest, if necessary */
@@ -418,6 +415,14 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
         return false;
     }
 
+    /* If dataplane is (re-)enabled while the guest is running there could be
+     * block jobs that can conflict.
+     */
+    if (bdrv_in_use(blk->conf.bs)) {
+        error_report("cannot start dataplane thread while device is in use");
+        return false;
+    }
+
     fd = raw_get_aio_fd(blk->conf.bs);
     if (fd < 0) {
         error_report("drive is incompatible with x-data-plane, "
@@ -433,10 +438,6 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
     /* Prevent block operations that conflict with data plane thread */
     bdrv_set_in_use(blk->conf.bs, 1);
 
-    error_setg(&s->migration_blocker,
-            "x-data-plane does not support migration");
-    migrate_add_blocker(s->migration_blocker);
-
     *dataplane = s;
     return true;
 }
@@ -448,8 +449,6 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
     }
 
     virtio_blk_data_plane_stop(s);
-    migrate_del_blocker(s->migration_blocker);
-    error_free(s->migration_blocker);
     bdrv_set_in_use(s->blk->conf.bs, 0);
     g_free(s);
 }
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index cf12469..cca0c77 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -19,6 +19,7 @@
 #include "hw/virtio/virtio-blk.h"
 #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
 # include "dataplane/virtio-blk.h"
+# include "migration/migration.h"
 #endif
 #include "block/scsi.h"
 #ifdef __linux__
@@ -628,6 +629,34 @@ void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk)
     memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf));
 }
 
+#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
+/* Disable dataplane thread during live migration since it does not
+ * update the dirty memory bitmap yet.
+ */
+static void virtio_blk_migration_state_changed(Notifier *notifier, void *data)
+{
+    VirtIOBlock *s = container_of(notifier, VirtIOBlock,
+                                  migration_state_notifier);
+    MigrationState *mig = data;
+
+    if (migration_is_active(mig)) {
+        if (!s->dataplane) {
+            return;
+        }
+        virtio_blk_data_plane_destroy(s->dataplane);
+        s->dataplane = NULL;
+    } else if (migration_has_finished(mig) ||
+               migration_has_failed(mig)) {
+        if (s->dataplane) {
+            return;
+        }
+        bdrv_drain_all(); /* complete in-flight non-dataplane requests */
+        virtio_blk_data_plane_create(VIRTIO_DEVICE(s), &s->blk,
+                                     &s->dataplane);
+    }
+}
+#endif /* CONFIG_VIRTIO_BLK_DATA_PLANE */
+
 static int virtio_blk_device_init(VirtIODevice *vdev)
 {
     DeviceState *qdev = DEVICE(vdev);
@@ -664,6 +693,8 @@ static int virtio_blk_device_init(VirtIODevice *vdev)
         virtio_cleanup(vdev);
         return -1;
     }
+    s->migration_state_notifier.notify = virtio_blk_migration_state_changed;
+    add_migration_state_change_notifier(&s->migration_state_notifier);
 #endif
 
     s->change = qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
@@ -683,6 +714,7 @@ static int virtio_blk_device_exit(DeviceState *dev)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOBlock *s = VIRTIO_BLK(dev);
 #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
+    remove_migration_state_change_notifier(&s->migration_state_notifier);
     virtio_blk_data_plane_destroy(s->dataplane);
     s->dataplane = NULL;
 #endif
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index fc71853..b87cf49 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -125,6 +125,7 @@ typedef struct VirtIOBlock {
     unsigned short sector_mask;
     VMChangeStateEntry *change;
 #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
+    Notifier migration_state_notifier;
     struct VirtIOBlockDataPlane *dataplane;
 #endif
 } VirtIOBlock;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 3/3] dataplane: enable virtio-blk x-data-plane=on live migration

[Paolo Bonzini]

Il 17/07/2013 11:35, Stefan Hajnoczi ha scritto:
> Although the dataplane thread does not cooperate with dirty memory
> logging yet it's fairly easy to temporarily disable dataplane during
> live migration.  This way virtio-blk can live migrate when
> x-data-plane=on.
> 
> The dataplane thread will restart after migration is cancelled or if the
> guest resuming virtio-blk operation after migration completes.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/block/dataplane/virtio-blk.c | 17 ++++++++---------
>  hw/block/virtio-blk.c           | 32 ++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio-blk.h  |  1 +
>  3 files changed, 41 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 2faed43..411becc 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -18,7 +18,6 @@
>  #include "qemu/error-report.h"
>  #include "hw/virtio/dataplane/vring.h"
>  #include "ioq.h"
> -#include "migration/migration.h"
>  #include "block/block.h"
>  #include "hw/virtio/virtio-blk.h"
>  #include "virtio-blk.h"
> @@ -69,8 +68,6 @@ struct VirtIOBlockDataPlane {
>                                               queue */
>  
>      unsigned int num_reqs;
> -
> -    Error *migration_blocker;
>  };
>  
>  /* Raise an interrupt to signal guest, if necessary */
> @@ -418,6 +415,14 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
>          return false;
>      }
>  
> +    /* If dataplane is (re-)enabled while the guest is running there could be
> +     * block jobs that can conflict.
> +     */
> +    if (bdrv_in_use(blk->conf.bs)) {
> +        error_report("cannot start dataplane thread while device is in use");
> +        return false;
> +    }
> +
>      fd = raw_get_aio_fd(blk->conf.bs);
>      if (fd < 0) {
>          error_report("drive is incompatible with x-data-plane, "
> @@ -433,10 +438,6 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
>      /* Prevent block operations that conflict with data plane thread */
>      bdrv_set_in_use(blk->conf.bs, 1);
>  
> -    error_setg(&s->migration_blocker,
> -            "x-data-plane does not support migration");
> -    migrate_add_blocker(s->migration_blocker);
> -
>      *dataplane = s;
>      return true;
>  }
> @@ -448,8 +449,6 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
>      }
>  
>      virtio_blk_data_plane_stop(s);
> -    migrate_del_blocker(s->migration_blocker);
> -    error_free(s->migration_blocker);
>      bdrv_set_in_use(s->blk->conf.bs, 0);
>      g_free(s);
>  }
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index cf12469..cca0c77 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -19,6 +19,7 @@
>  #include "hw/virtio/virtio-blk.h"
>  #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
>  # include "dataplane/virtio-blk.h"
> +# include "migration/migration.h"
>  #endif
>  #include "block/scsi.h"
>  #ifdef __linux__
> @@ -628,6 +629,34 @@ void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk)
>      memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf));
>  }
>  
> +#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
> +/* Disable dataplane thread during live migration since it does not
> + * update the dirty memory bitmap yet.
> + */
> +static void virtio_blk_migration_state_changed(Notifier *notifier, void *data)
> +{
> +    VirtIOBlock *s = container_of(notifier, VirtIOBlock,
> +                                  migration_state_notifier);
> +    MigrationState *mig = data;
> +
> +    if (migration_is_active(mig)) {
> +        if (!s->dataplane) {
> +            return;
> +        }
> +        virtio_blk_data_plane_destroy(s->dataplane);
> +        s->dataplane = NULL;
> +    } else if (migration_has_finished(mig) ||
> +               migration_has_failed(mig)) {
> +        if (s->dataplane) {
> +            return;
> +        }
> +        bdrv_drain_all(); /* complete in-flight non-dataplane requests */
> +        virtio_blk_data_plane_create(VIRTIO_DEVICE(s), &s->blk,
> +                                     &s->dataplane);
> +    }
> +}

Perhaps you can call bdrv_set_in_use here (set it to 1 after
destruction, and to 0 before creation), so that you do not need the
check in virtio_blk_data_plane_create?

> +#endif /* CONFIG_VIRTIO_BLK_DATA_PLANE */
> +
>  static int virtio_blk_device_init(VirtIODevice *vdev)
>  {
>      DeviceState *qdev = DEVICE(vdev);
> @@ -664,6 +693,8 @@ static int virtio_blk_device_init(VirtIODevice *vdev)
>          virtio_cleanup(vdev);
>          return -1;
>      }
> +    s->migration_state_notifier.notify = virtio_blk_migration_state_changed;
> +    add_migration_state_change_notifier(&s->migration_state_notifier);
>  #endif
>  
>      s->change = qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
> @@ -683,6 +714,7 @@ static int virtio_blk_device_exit(DeviceState *dev)
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VirtIOBlock *s = VIRTIO_BLK(dev);
>  #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
> +    remove_migration_state_change_notifier(&s->migration_state_notifier);
>      virtio_blk_data_plane_destroy(s->dataplane);
>      s->dataplane = NULL;
>  #endif
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index fc71853..b87cf49 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -125,6 +125,7 @@ typedef struct VirtIOBlock {
>      unsigned short sector_mask;
>      VMChangeStateEntry *change;
>  #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
> +    Notifier migration_state_notifier;
>      struct VirtIOBlockDataPlane *dataplane;
>  #endif
>  } VirtIOBlock;
> 

Only a stopgap measure, but it's self-contained and easy to revert,
which makes it a brilliant solution.  Just one nit above to make it even
more self-contained.

Paolo

Re: [Qemu-devel] [PATCH 3/3] dataplane: enable virtio-blk x-data-plane=on live migration

[Stefan Hajnoczi]

On Wed, Jul 17, 2013 at 12:26:54PM +0200, Paolo Bonzini wrote:
> Il 17/07/2013 11:35, Stefan Hajnoczi ha scritto:
> > @@ -628,6 +629,34 @@ void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk)
> >      memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf));
> >  }
> >  
> > +#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
> > +/* Disable dataplane thread during live migration since it does not
> > + * update the dirty memory bitmap yet.
> > + */
> > +static void virtio_blk_migration_state_changed(Notifier *notifier, void *data)
> > +{
> > +    VirtIOBlock *s = container_of(notifier, VirtIOBlock,
> > +                                  migration_state_notifier);
> > +    MigrationState *mig = data;
> > +
> > +    if (migration_is_active(mig)) {
> > +        if (!s->dataplane) {
> > +            return;
> > +        }
> > +        virtio_blk_data_plane_destroy(s->dataplane);
> > +        s->dataplane = NULL;
> > +    } else if (migration_has_finished(mig) ||
> > +               migration_has_failed(mig)) {
> > +        if (s->dataplane) {
> > +            return;
> > +        }
> > +        bdrv_drain_all(); /* complete in-flight non-dataplane requests */
> > +        virtio_blk_data_plane_create(VIRTIO_DEVICE(s), &s->blk,
> > +                                     &s->dataplane);
> > +    }
> > +}
> 
> Perhaps you can call bdrv_set_in_use here (set it to 1 after
> destruction, and to 0 before creation), so that you do not need the
> check in virtio_blk_data_plane_create?

The bdrv_in_use() check fixes a bug that was present before this series.
Therefore I split it into a separate commit in v3 and CCed
qemu-stable@nongnu.org.

Stefan