[Qemu-devel] [PATCH v4 0/4] Don't write headers if BDS is INACTIVE

[Qemu-devel] [PATCH v4 0/4] Don't write headers if BDS is INACTIVE

[Jeff Cody]

Changes from v3->v4:

Patch 3: Add migrate_del_blocker and error_free (Thanks Stefan)


git-backport-diff -r qemu/master.. -u ba11b69

001/4:[----] [--] 'block/vhdx.c: Don't blindly update the header'
002/4:[----] [--] 'block/parallels: Do not update header or truncate image when INMIGRATE'
003/4:[0003] [FC] 'block/parallels: add migration blocker'
004/4:[----] [--] 'qemu-iotests: update unsupported image formats in 194'


Changes from v2->v3:

Patch 2: Uh... fix that misspelling.  Thanks Stefan :)
Patch 3: New patch to block migration in parallels

git-backport-diff -r qemu/master.. -u 6dc6acb

001/4:[----] [--] 'block/vhdx.c: Don't blindly update the header'
002/4:[----] [--] 'block/parallels: Do not update header or truncate image when INMIGRATE'
003/4:[down] 'block/parallels: add migration blocker'
004/4:[----] [--] 'qemu-iotests: update unsupported image formats in 194'


Changes from v1->v2:

* Drop previous parallels patches, just check BDRV_O_INACTIVE now
  (Kevin)

git-backport-diff -r qemu/master.. -u github/master
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/3:[----] [--] 'block/vhdx.c: Don't blindly update the header'
002/3:[down] 'block/parallals: Do not update header or truncate image when INMIGRATE'
003/3:[----] [--] 'qemu-iotests: update unsupported image formats in 194'


v1:

VHDX and Parallels both blindly write headers to the image file
if the images are opened R/W.  This causes an assert if the QEMU run
state is INMIGRATE.

Jeff Cody (4):
  block/vhdx.c: Don't blindly update the header
  block/parallels: Do not update header or truncate image when INMIGRATE
  block/parallels: add migration blocker
  qemu-iotests: update unsupported image formats in 194

 block/parallels.c      | 22 +++++++++++++++++-----
 block/vhdx.c           |  7 -------
 tests/qemu-iotests/194 |  2 +-
 3 files changed, 18 insertions(+), 13 deletions(-)

-- 
2.9.5

[Qemu-devel] [PATCH v4 1/4] block/vhdx.c: Don't blindly update the header

[Jeff Cody]

The VHDX specification requires that before user data modification of
the vhdx image, the VHDX header file and data GUIDs need to be updated.
In vhdx_open(), if the image is set to RDWR, we go ahead and update the
header.

However, just because the image is set to RDWR does not mean we can go
ahead and write at this point - specifically, if the QEMU run state is
INMIGRATE, the underlying file BS may be set to inactive via the BDS
open flag of BDRV_O_INACTIVE.  Attempting to write under this condition
will cause an assert in bdrv_co_pwritev().

We can alternatively latch the first time the image is written.  And lo
and behold, we do just that, via vhdx_user_visible_write() in
vhdx_co_writev().  This means the call to vhdx_update_headers() in
vhdx_open() is likely just vestigial, and can be removed.

Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Tested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vhdx.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/block/vhdx.c b/block/vhdx.c
index 7ae4589..9956933 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1008,13 +1008,6 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    if (flags & BDRV_O_RDWR) {
-        ret = vhdx_update_headers(bs, s, false, NULL);
-        if (ret < 0) {
-            goto fail;
-        }
-    }
-
     /* TODO: differencing files */
 
     return 0;
-- 
2.9.5

[Qemu-devel] [PATCH v4 2/4] block/parallels: Do not update header or truncate image when INMIGRATE

[Jeff Cody]

If we write or modify the image file while the QEMU run state is
INMIGRATE, then the BDRV_O_INACTIVE BDS flag is set.  This will cause
an assert, since the image is marked inactive.  Make sure we obey this
flag.

Tested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/parallels.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 2b6c6e5..7b7a3ef 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -708,7 +708,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
     }
 
-    if (flags & BDRV_O_RDWR) {
+    if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_INACTIVE)) {
         s->header->inuse = cpu_to_le32(HEADER_INUSE_MAGIC);
         ret = parallels_update_header(bs);
         if (ret < 0) {
@@ -741,12 +741,9 @@ static void parallels_close(BlockDriverState *bs)
 {
     BDRVParallelsState *s = bs->opaque;
 
-    if (bs->open_flags & BDRV_O_RDWR) {
+    if ((bs->open_flags & BDRV_O_RDWR) && !(bs->open_flags & BDRV_O_INACTIVE)) {
         s->header->inuse = 0;
         parallels_update_header(bs);
-    }
-
-    if (bs->open_flags & BDRV_O_RDWR) {
         bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS,
                       PREALLOC_MODE_OFF, NULL);
     }
-- 
2.9.5

[Qemu-devel] [PATCH v4 3/4] block/parallels: add migration blocker

[Jeff Cody]

Migration does not work for parallels, and has been broken for a while
(see patch 'block/parallels: Do not update header or truncate image when
 INMIGRATE').  The bdrv_invalidate_cache() method needs to be added for
migration to be supported.  Until this is done, prohibit migration.

Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
 block/parallels.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 7b7a3ef..9545761 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -35,6 +35,7 @@
 #include "qemu/module.h"
 #include "qemu/bswap.h"
 #include "qemu/bitmap.h"
+#include "migration/blocker.h"
 
 /**************************************************************/
 
@@ -100,6 +101,7 @@ typedef struct BDRVParallelsState {
     unsigned int tracks;
 
     unsigned int off_multiplier;
+    Error *migration_blocker;
 } BDRVParallelsState;
 
 
@@ -720,6 +722,16 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     s->bat_dirty_bmap =
         bitmap_new(DIV_ROUND_UP(s->header_size, s->bat_dirty_block));
 
+    /* Disable migration until bdrv_invalidate_cache method is added */
+    error_setg(&s->migration_blocker, "The Parallels format used by node '%s' "
+               "does not support live migration",
+               bdrv_get_device_or_node_name(bs));
+    ret = migrate_add_blocker(s->migration_blocker, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        error_free(s->migration_blocker);
+        goto fail;
+    }
     qemu_co_mutex_init(&s->lock);
     return 0;
 
@@ -750,6 +762,9 @@ static void parallels_close(BlockDriverState *bs)
 
     g_free(s->bat_dirty_bmap);
     qemu_vfree(s->header);
+
+    migrate_del_blocker(s->migration_blocker);
+    error_free(s->migration_blocker);
 }
 
 static QemuOptsList parallels_create_opts = {
-- 
2.9.5

[Qemu-devel] [PATCH v4 4/4] qemu-iotests: update unsupported image formats in 194

[Jeff Cody]

Test 194 checks for 'luks' to exclude as an unsupported format,
However, most formats are unsupported, due to migration blockers.

Rather than specifying a blacklist of unsupported formats, whitelist
supported formats (specifically, qcow2, qed, raw, dmg).

Tested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 tests/qemu-iotests/194 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index 8d973b4..1d4214a 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -21,7 +21,7 @@
 
 import iotests
 
-iotests.verify_image_format(unsupported_fmts=['luks'])
+iotests.verify_image_format(supported_fmts=['qcow2', 'qed', 'raw', 'dmg'])
 iotests.verify_platform(['linux'])
 
 with iotests.FilePath('source.img') as source_img_path, \
-- 
2.9.5

Re: [Qemu-devel] [Qemu-block] [PATCH v4 0/4] Don't write headers if BDS is INACTIVE

[Stefan Hajnoczi]

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

On Tue, Nov 07, 2017 at 08:10:32AM -0500, Jeff Cody wrote:
> Changes from v3->v4:
> 
> Patch 3: Add migrate_del_blocker and error_free (Thanks Stefan)
> 
> 
> git-backport-diff -r qemu/master.. -u ba11b69
> 
> 001/4:[----] [--] 'block/vhdx.c: Don't blindly update the header'
> 002/4:[----] [--] 'block/parallels: Do not update header or truncate image when INMIGRATE'
> 003/4:[0003] [FC] 'block/parallels: add migration blocker'
> 004/4:[----] [--] 'qemu-iotests: update unsupported image formats in 194'
> 
> 
> Changes from v2->v3:
> 
> Patch 2: Uh... fix that misspelling.  Thanks Stefan :)
> Patch 3: New patch to block migration in parallels
> 
> git-backport-diff -r qemu/master.. -u 6dc6acb
> 
> 001/4:[----] [--] 'block/vhdx.c: Don't blindly update the header'
> 002/4:[----] [--] 'block/parallels: Do not update header or truncate image when INMIGRATE'
> 003/4:[down] 'block/parallels: add migration blocker'
> 004/4:[----] [--] 'qemu-iotests: update unsupported image formats in 194'
> 
> 
> Changes from v1->v2:
> 
> * Drop previous parallels patches, just check BDRV_O_INACTIVE now
>   (Kevin)
> 
> git-backport-diff -r qemu/master.. -u github/master
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/3:[----] [--] 'block/vhdx.c: Don't blindly update the header'
> 002/3:[down] 'block/parallals: Do not update header or truncate image when INMIGRATE'
> 003/3:[----] [--] 'qemu-iotests: update unsupported image formats in 194'
> 
> 
> v1:
> 
> VHDX and Parallels both blindly write headers to the image file
> if the images are opened R/W.  This causes an assert if the QEMU run
> state is INMIGRATE.
> 
> Jeff Cody (4):
>   block/vhdx.c: Don't blindly update the header
>   block/parallels: Do not update header or truncate image when INMIGRATE
>   block/parallels: add migration blocker
>   qemu-iotests: update unsupported image formats in 194
> 
>  block/parallels.c      | 22 +++++++++++++++++-----
>  block/vhdx.c           |  7 -------
>  tests/qemu-iotests/194 |  2 +-
>  3 files changed, 18 insertions(+), 13 deletions(-)
> 
> -- 
> 2.9.5
> 
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v4 0/4] Don't write headers if BDS is INACTIVE

[Denis V. Lunev]

On 11/07/2017 04:10 PM, Jeff Cody wrote:
> Changes from v3->v4:
>
> Patch 3: Add migrate_del_blocker and error_free (Thanks Stefan)
>
>
> git-backport-diff -r qemu/master.. -u ba11b69
>
> 001/4:[----] [--] 'block/vhdx.c: Don't blindly update the header'
> 002/4:[----] [--] 'block/parallels: Do not update header or truncate image when INMIGRATE'
> 003/4:[0003] [FC] 'block/parallels: add migration blocker'
> 004/4:[----] [--] 'qemu-iotests: update unsupported image formats in 194'
>
>
> Changes from v2->v3:
>
> Patch 2: Uh... fix that misspelling.  Thanks Stefan :)
> Patch 3: New patch to block migration in parallels
>
> git-backport-diff -r qemu/master.. -u 6dc6acb
>
> 001/4:[----] [--] 'block/vhdx.c: Don't blindly update the header'
> 002/4:[----] [--] 'block/parallels: Do not update header or truncate image when INMIGRATE'
> 003/4:[down] 'block/parallels: add migration blocker'
> 004/4:[----] [--] 'qemu-iotests: update unsupported image formats in 194'
>
>
> Changes from v1->v2:
>
> * Drop previous parallels patches, just check BDRV_O_INACTIVE now
>   (Kevin)
>
> git-backport-diff -r qemu/master.. -u github/master
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
>
> 001/3:[----] [--] 'block/vhdx.c: Don't blindly update the header'
> 002/3:[down] 'block/parallals: Do not update header or truncate image when INMIGRATE'
> 003/3:[----] [--] 'qemu-iotests: update unsupported image formats in 194'
>
>
> v1:
>
> VHDX and Parallels both blindly write headers to the image file
> if the images are opened R/W.  This causes an assert if the QEMU run
> state is INMIGRATE.
>
> Jeff Cody (4):
>   block/vhdx.c: Don't blindly update the header
>   block/parallels: Do not update header or truncate image when INMIGRATE
>   block/parallels: add migration blocker
>   qemu-iotests: update unsupported image formats in 194
>
>  block/parallels.c      | 22 +++++++++++++++++-----
>  block/vhdx.c           |  7 -------
>  tests/qemu-iotests/194 |  2 +-
>  3 files changed, 18 insertions(+), 13 deletions(-)
>
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [Qemu-devel] [PATCH v4 1/4] block/vhdx.c: Don't blindly update the header

[Max Reitz]

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

On 2017-11-07 14:10, Jeff Cody wrote:
> The VHDX specification requires that before user data modification of
> the vhdx image, the VHDX header file and data GUIDs need to be updated.
> In vhdx_open(), if the image is set to RDWR, we go ahead and update the
> header.
> 
> However, just because the image is set to RDWR does not mean we can go
> ahead and write at this point - specifically, if the QEMU run state is
> INMIGRATE, the underlying file BS may be set to inactive via the BDS
> open flag of BDRV_O_INACTIVE.  Attempting to write under this condition
> will cause an assert in bdrv_co_pwritev().
> 
> We can alternatively latch the first time the image is written.  And lo
> and behold, we do just that, via vhdx_user_visible_write() in
> vhdx_co_writev().  This means the call to vhdx_update_headers() in
> vhdx_open() is likely just vestigial, and can be removed.
> 
> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Tested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/vhdx.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 7ae4589..9956933 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1008,13 +1008,6 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail;
>      }
>  
> -    if (flags & BDRV_O_RDWR) {
> -        ret = vhdx_update_headers(bs, s, false, NULL);

And this doesn't even update the data GUID...

Max

> -        if (ret < 0) {
> -            goto fail;
> -        }
> -    }
> -
>      /* TODO: differencing files */
>  
>      return 0;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v4 0/4] Don't write headers if BDS is INACTIVE

[Max Reitz]

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

On 2017-11-07 14:10, Jeff Cody wrote:
> Changes from v3->v4:
> 
> Patch 3: Add migrate_del_blocker and error_free (Thanks Stefan)

Thanks, applied to my block branch:

https://github.com/XanClic/qemu/commits/block

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]