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

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

[Jeff Cody]

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 (3):
  block/vhdx.c: Don't blindly update the header
  block/parallals: Do not update header or truncate image when INMIGRATE
  qemu-iotests: update unsupported image formats in 194

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

-- 
2.13.6

[Qemu-devel] [PATCH v2 1/3] 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>
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 7ae4589879..9956933da6 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.13.6

[Qemu-devel] [PATCH v2 2/3] block/parallals: 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.

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 2b6c6e5709..7b7a3efa1d 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.13.6

Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/3] block/parallals: Do not update header or truncate image when INMIGRATE

[Stefan Hajnoczi]

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

On Mon, Oct 30, 2017 at 02:10:27PM +0100, Jeff Cody wrote:
> 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.
> 
> 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 2b6c6e5709..7b7a3efa1d 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) {

Where do we ensure that HEADER_INUSE_MAGIC is set on the destination?

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

Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/3] block/parallals: Do not update header or truncate image when INMIGRATE

[Denis V. Lunev]

On 11/03/2017 02:05 PM, Stefan Hajnoczi wrote:
> On Mon, Oct 30, 2017 at 02:10:27PM +0100, Jeff Cody wrote:
>> 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.
>>
>> 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 2b6c6e5709..7b7a3efa1d 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) {
> Where do we ensure that HEADER_INUSE_MAGIC is set on the destination?
good point. it should be set in invalidate_cache following QCOW2 scheme.
Thus we should add that callback with this patch.

Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/3] block/parallals: Do not update header or truncate image when INMIGRATE

[Jeff Cody]

On Fri, Nov 03, 2017 at 02:08:13PM +0300, Denis V. Lunev wrote:
> On 11/03/2017 02:05 PM, Stefan Hajnoczi wrote:
> > On Mon, Oct 30, 2017 at 02:10:27PM +0100, Jeff Cody wrote:
> >> 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.
> >>
> >> 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 2b6c6e5709..7b7a3efa1d 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) {
> > Where do we ensure that HEADER_INUSE_MAGIC is set on the destination?
> good point. it should be set in invalidate_cache following QCOW2 scheme.
> Thus we should add that callback with this patch.
> 


Do we (intentionally) support migration with parallels?  I guess so, since
it doesn't have migration blockers.

Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/3] block/parallals: Do not update header or truncate image when INMIGRATE

[Jeff Cody]

On Mon, Nov 06, 2017 at 03:23:43PM -0500, Jeff Cody wrote:
> On Fri, Nov 03, 2017 at 02:08:13PM +0300, Denis V. Lunev wrote:
> > On 11/03/2017 02:05 PM, Stefan Hajnoczi wrote:
> > > On Mon, Oct 30, 2017 at 02:10:27PM +0100, Jeff Cody wrote:
> > >> 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.
> > >>
> > >> 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 2b6c6e5709..7b7a3efa1d 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) {
> > > Where do we ensure that HEADER_INUSE_MAGIC is set on the destination?
> > good point. it should be set in invalidate_cache following QCOW2 scheme.
> > Thus we should add that callback with this patch.
> > 
> 
> 
> Do we (intentionally) support migration with parallels?  I guess so, since
> it doesn't have migration blockers.

Hmm, I hit send prematurely...

Despite not having migration blockers, obviously nobody is using migration
with parallels; from v2.6.0 to now, attempting to do so causes QEMU to
assert due to the bug fixed in this patch.

Rather than implement .bdrv_invalidate_cache() in this series, I am going to
set a migration blocker in a separate patch (in this series), so that
migration is not supported.  Whenever .bdrv_invalidate_cache() is
implemented, then the blocker can be removed.

-Jeff

[Qemu-devel] [PATCH v2 3/3] 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).

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 8d973b440f..1d4214aca3 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.13.6

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

[Alexey Kardashevskiy]

On 31/10/17 00:10, Jeff Cody wrote:
> 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'



Tested-by: Alexey Kardashevskiy <aik@ozlabs.ru>


> 
> 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 (3):
>   block/vhdx.c: Don't blindly update the header
>   block/parallals: Do not update header or truncate image when INMIGRATE
>   qemu-iotests: update unsupported image formats in 194
> 
>  block/parallels.c      | 7 ++-----
>  block/vhdx.c           | 7 -------
>  tests/qemu-iotests/194 | 2 +-
>  3 files changed, 3 insertions(+), 13 deletions(-)
> 


-- 
Alexey

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

[Stefan Hajnoczi]

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

On Mon, Oct 30, 2017 at 02:10:25PM +0100, Jeff Cody wrote:
> 
> 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'

parallals?!  What happened here?  :)

> 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 (3):
>   block/vhdx.c: Don't blindly update the header
>   block/parallals: Do not update header or truncate image when INMIGRATE
>   qemu-iotests: update unsupported image formats in 194
> 
>  block/parallels.c      | 7 ++-----
>  block/vhdx.c           | 7 -------
>  tests/qemu-iotests/194 | 2 +-
>  3 files changed, 3 insertions(+), 13 deletions(-)
> 
> -- 
> 2.13.6
> 
> 

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