diff mbox

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

Message ID 1374053720-28420-4-git-send-email-stefanha@redhat.com
State New
Headers show

Commit Message

Stefan Hajnoczi July 17, 2013, 9:35 a.m. UTC
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(-)

Comments

Paolo Bonzini July 17, 2013, 10:26 a.m. UTC | #1
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
Stefan Hajnoczi July 23, 2013, 1:39 p.m. UTC | #2
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
diff mbox

Patch

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;