[Qemu-devel] [PATCH V2 0/7] Make QED with live migration safe

[Qemu-devel] [PATCH V2 0/7] Make QED with live migration safe

[Benoît Canet]

This is the second version of a patchset aiming at making the combined
usage of QED and live migration safe.

Since v1:

-The block layer is not aware anymore of the migration state. (stefanha)
-No bdrv_invalidate_cache renaming since the semantic do not change. (stefanha)
-The qed bdrv_invalidate_cache function does a reopening of the image to flush
 metadata and to do the image integrity check. (stefanha)

note:

In "qed: honor BDRV_O_INCOMING for incoming live migration" I choose to
honor bs->open_flags and not s->flags in order to keep the
bdrv_qed_invalidate_cache function intact of any migration related operation.
This way bdrv_qed_invalidate_cache semantic is not changed.

Benoît Canet (7):
  block: Add new BDRV_O_INCOMING flag to notice incoming live migration
  block: add a function to clear incoming live migration flags
  blockdev: open images with BDRV_O_INCOMING on incoming live migration
  qed: add bdrv_invalidate_cache to be called after incoming live
    migration
  migration: clear BDRV_O_INCOMING flags on end of incoming live
    migration
  qed: honor BDRV_O_INCOMING for incoming live migration
  qed: remove incoming live migration blocker

 block.c     |    9 +++++++++
 block.h     |    3 +++
 block/qed.c |   29 ++++++++++++++++++-----------
 block/qed.h |    3 +--
 blockdev.c  |    4 ++++
 migration.c |    1 +
 6 files changed, 36 insertions(+), 13 deletions(-)

-- 
1.7.7.6

[Qemu-devel] [PATCH V2 1/7] block: Add new BDRV_O_INCOMING flag to notice incoming live migration

[Benoît Canet]

>From original patch with Patchwork-id: 31110 by
Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

"Add a flag to indicate that incoming migration is pending and care needs
to be taken for data consistency.  Block drivers should not modify the
image file before incoming migration is complete since the migration
source host is still using the image file."

The rationale for not using bdrv->read_only is the following.

"Unfortunately this is not possible because too many other places in QEMU
test bdrv_is_read_only() and use it for their own evil purposes.  For
example, ide_init_drive() will error out because read-only harddisks are
not supported.  We're mixing guest and host side read-only concepts so
this simpler alternative does not work."

Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
---
 block.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/block.h b/block.h
index 415bb17..b3b18d6 100644
--- a/block.h
+++ b/block.h
@@ -71,6 +71,7 @@ typedef struct BlockDevOps {
 #define BDRV_O_NO_BACKING  0x0100 /* don't open the backing file */
 #define BDRV_O_NO_FLUSH    0x0200 /* disable flushing on this disk */
 #define BDRV_O_COPY_ON_READ 0x0400 /* copy read backing sectors into image */
+#define BDRV_O_INCOMING    0x0800  /* consistency hint for incoming migration */
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
 
-- 
1.7.7.6

[Qemu-devel] [PATCH V2 2/7] block: add a function to clear incoming live migration flags

[Benoît Canet]

This function will clear all BDRV_O_INCOMING flags.

Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
---
 block.c |    9 +++++++++
 block.h |    2 ++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index b88ee90..45085e7 100644
--- a/block.c
+++ b/block.c
@@ -3584,6 +3584,15 @@ void bdrv_invalidate_cache_all(void)
     }
 }
 
+void bdrv_clear_incoming_migration_all(void)
+{
+    BlockDriverState *bs;
+
+    QTAILQ_FOREACH(bs, &bdrv_states, list) {
+        bs->open_flags = bs->open_flags & ~(BDRV_O_INCOMING);
+    }
+}
+
 int bdrv_flush(BlockDriverState *bs)
 {
     Coroutine *co;
diff --git a/block.h b/block.h
index b3b18d6..951b476 100644
--- a/block.h
+++ b/block.h
@@ -223,6 +223,8 @@ BlockDriverAIOCB *bdrv_aio_ioctl(BlockDriverState *bs,
 void bdrv_invalidate_cache(BlockDriverState *bs);
 void bdrv_invalidate_cache_all(void);
 
+void bdrv_clear_incoming_migration_all(void);
+
 /* Ensure contents are flushed to disk.  */
 int bdrv_flush(BlockDriverState *bs);
 int coroutine_fn bdrv_co_flush(BlockDriverState *bs);
-- 
1.7.7.6

[Qemu-devel] [PATCH V2 3/7] blockdev: open images with BDRV_O_INCOMING on incoming live migration

[Benoît Canet]

Open images with BDRV_O_INCOMING in order to inform block drivers
that an incoming live migration is coming.

Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
---
 blockdev.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 1a500b8..9b57133 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -591,6 +591,10 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
         bdrv_flags |= BDRV_O_COPY_ON_READ;
     }
 
+    if (runstate_check(RUN_STATE_INMIGRATE)) {
+        bdrv_flags |= BDRV_O_INCOMING;
+    }
+
     if (media == MEDIA_CDROM) {
         /* CDROM is fine for any interface, don't check.  */
         ro = 1;
-- 
1.7.7.6

[Qemu-devel] [PATCH V2 4/7] qed: add bdrv_invalidate_cache to be called after incoming live migration

[Benoît Canet]

The QED image is reopened to flush metadata and check consistency.

Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
---
 block/qed.c |   15 +++++++++++++++
 block/qed.h |    1 +
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index a041d31..c47272c 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -375,6 +375,10 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
     int ret;
 
     s->bs = bs;
+
+    /* backup flags for bdrv_qed_invalidate_cache */
+    s->flags = flags;
+
     QSIMPLEQ_INIT(&s->allocating_write_reqs);
 
     ret = bdrv_pread(bs->file, 0, &le_header, sizeof(le_header));
@@ -1516,6 +1520,16 @@ static int bdrv_qed_change_backing_file(BlockDriverState *bs,
     return ret;
 }
 
+static void bdrv_qed_invalidate_cache(BlockDriverState *bs)
+{
+    BDRVQEDState *s = bs->opaque;
+    int flags = s->flags;
+
+    bdrv_qed_close(bs);
+    memset(s, 0, sizeof(BDRVQEDState));
+    bdrv_qed_open(bs, flags);
+}
+
 static int bdrv_qed_check(BlockDriverState *bs, BdrvCheckResult *result)
 {
     BDRVQEDState *s = bs->opaque;
@@ -1568,6 +1582,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_getlength           = bdrv_qed_getlength,
     .bdrv_get_info            = bdrv_qed_get_info,
     .bdrv_change_backing_file = bdrv_qed_change_backing_file,
+    .bdrv_invalidate_cache    = bdrv_qed_invalidate_cache,
     .bdrv_check               = bdrv_qed_check,
 };
 
diff --git a/block/qed.h b/block/qed.h
index 62624a1..cb1ebd8 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -153,6 +153,7 @@ typedef struct QEDAIOCB {
 
 typedef struct {
     BlockDriverState *bs;           /* device */
+    int flags;                      /* open flags */
     uint64_t file_size;             /* length of image file, in bytes */
 
     QEDHeader header;               /* always cpu-endian */
-- 
1.7.7.6

[Qemu-devel] [PATCH V2 5/7] migration: clear BDRV_O_INCOMING flags on end of incoming live migration

[Benoît Canet]

Signed-off-by: Benoît Canet <benoit.canet@gmail.com>
---
 migration.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/migration.c b/migration.c
index 8c119ba..94f7839 100644
--- a/migration.c
+++ b/migration.c
@@ -91,6 +91,7 @@ void process_incoming_migration(QEMUFile *f)
     qemu_announce_self();
     DPRINTF("successfully loaded vm state\n");
 
+    bdrv_clear_incoming_migration_all();
     /* Make sure all file formats flush their mutable metadata */
     bdrv_invalidate_cache_all();
 
-- 
1.7.7.6

[Qemu-devel] [PATCH V2 6/7] qed: honor BDRV_O_INCOMING for incoming live migration

[Benoît Canet]

>From original commit with Patchwork-id: 31108 by
Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

"The QED image format includes a file header bit to mark images dirty.
QED normally checks dirty images on open and fixes inconsistent
metadata.  This is undesirable during live migration since the dirty bit
may be set if the source host is modifying the image file.  The check
should be postponed until migration completes.

Skip operations that modify the image file if the BDRV_O_INCOMING flag
is set."

Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
---
 block/qed.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index c47272c..4c04bc9 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -454,7 +454,7 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
      * feature is no longer valid.
      */
     if ((s->header.autoclear_features & ~QED_AUTOCLEAR_FEATURE_MASK) != 0 &&
-        !bdrv_is_read_only(bs->file)) {
+        !bdrv_is_read_only(bs->file) && !(bs->open_flags & BDRV_O_INCOMING)) {
         s->header.autoclear_features &= QED_AUTOCLEAR_FEATURE_MASK;
 
         ret = qed_write_header_sync(s);
@@ -481,7 +481,8 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
          * potentially inconsistent images to be opened read-only.  This can
          * aid data recovery from an otherwise inconsistent image.
          */
-        if (!bdrv_is_read_only(bs->file)) {
+        if (!bdrv_is_read_only(bs->file) &&
+            !(bs->open_flags & BDRV_O_INCOMING)) {
             BdrvCheckResult result = {0};
 
             ret = qed_check(s, &result, true);
-- 
1.7.7.6

[Qemu-devel] [PATCH V2 7/7] qed: remove incoming live migration blocker

[Benoît Canet]

Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
---
 block/qed.c |    9 ---------
 block/qed.h |    2 --
 2 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 4c04bc9..803a6c2 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -502,12 +502,6 @@ 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);
@@ -520,9 +514,6 @@ 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 cb1ebd8..9fac300 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -170,8 +170,6 @@ typedef struct {
 
     /* Periodic flush and clear need check flag */
     QEMUTimer *need_check_timer;
-
-    Error *migration_blocker;
 } BDRVQEDState;
 
 enum {
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH V2 4/7] qed: add bdrv_invalidate_cache to be called after incoming live migration

[Stefan Hajnoczi]

On Wed, Mar 21, 2012 at 3:52 PM, Benoît Canet <benoit.canet@gmail.com> wrote:
> The QED image is reopened to flush metadata and check consistency.
>
> Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
> ---
>  block/qed.c |   15 +++++++++++++++
>  block/qed.h |    1 +
>  2 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/block/qed.c b/block/qed.c
> index a041d31..c47272c 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -375,6 +375,10 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
>     int ret;
>
>     s->bs = bs;
> +
> +    /* backup flags for bdrv_qed_invalidate_cache */
> +    s->flags = flags;

It's not clear to me why we need to introduce this field to stash
flags values.  bs->open_flags already has this information.

Originally this was introduced in 06d9260ffa9 ("qcow2: implement
bdrv_invalidate_cache (v2)") for qcow2.  I wonder if that field is
necessary when we already have bs->open_flags.

What I don't like about s->flags is that it duplicates state *and*
it's done in each block driver that supports .bdrv_invalidate_cache().
 So I wonder if we can drop it?

Stefan

Re: [Qemu-devel] [PATCH V2 4/7] qed: add bdrv_invalidate_cache to be called after incoming live migration

[Benoît Canet]

[-- Attachment #1: Type: text/plain, Size: 791 bytes --]

>It's not clear to me why we need to introduce this field to stash
>flags values.  bs->open_flags already has this information.

>Originally this was introduced in 06d9260ffa9 ("qcow2: implement
>bdrv_invalidate_cache (v2)") for qcow2.  I wonder if that field is
>necessary when we already have bs->open_flags.

>What I don't like about s->flags is that it duplicates state *and*
>it's done in each block driver that supports .bdrv_invalidate_cache().
> So I wonder if we can drop it?

I added this flag after seeing the following code in in bdrv_open_common.

    /*
     * Clear flags that are internal to the block layer before opening the
     * image.
     */
    open_flags = flags & ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);

This lead me to believe that  bs->open_flags != open_flags.

[-- Attachment #2: Type: text/html, Size: 1067 bytes --]

Re: [Qemu-devel] [PATCH V2 4/7] qed: add bdrv_invalidate_cache to be called after incoming live migration

[Paolo Bonzini]

Il 22/03/2012 15:15, Benoît Canet ha scritto:
> 
> I added this flag after seeing the following code in in bdrv_open_common.
> 
>     /*
>      * Clear flags that are internal to the block layer before opening the
>      * image.
>      */
>     open_flags = flags & ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
> 
> This lead me to believe that  bs->open_flags != open_flags.

Correct, see the first two patches in the series at
http://lists.gnu.org/archive/html/qemu-devel/2012-01/msg03467.html

Paolo

Re: [Qemu-devel] [PATCH V2 4/7] qed: add bdrv_invalidate_cache to be called after incoming live migration

[Stefan Hajnoczi]

On Thu, Mar 22, 2012 at 2:17 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 22/03/2012 15:15, Benoît Canet ha scritto:
>>
>> I added this flag after seeing the following code in in bdrv_open_common.
>>
>>     /*
>>      * Clear flags that are internal to the block layer before opening the
>>      * image.
>>      */
>>     open_flags = flags & ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>>
>> This lead me to believe that  bs->open_flags != open_flags.
>
> Correct, see the first two patches in the series at
> http://lists.gnu.org/archive/html/qemu-devel/2012-01/msg03467.html

We mangle the flags but I guess what I'm asking is, does it even
matter?  Those two bits don't affect qed or qcow2 .bdrv_open().  If
possible, I think we should use bs->open_flags.

Stefan