Don't write headers if BDS is INACTIVE
v2 cover Don't write headers if BDS is INACTIVE
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(-)
Signature · 1 line
2.13.6
PATCH v2 1/3 block/vhdx.c: Don't blindly update the header
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.
--- block/vhdx.c | 7 ------- 1 file changed, 7 deletions(-)
block/vhdx.c +0 -7
@@ -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 ;
Signature · 1 line
2.13.6
PATCH v2 2/3 block/parallals: Do not update header or truncate image when INMIGRATE
In reply to Jeff CodyIf 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.
--- block/parallels.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
block/parallels.c +2 -5
@@ -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 ); }
Signature · 1 line
2.13.6
Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/3] block/parallals: Do not update header or truncate image when INMIGRATE
Quoted 15 lines · Jeff Cody
If we write or modify the image file while the QEMU run state is INMIGRATE, then the
BDRV_O_INACTIVEBDS 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 <[email protected]>--- block/parallels.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)block/parallels.c +0
-0
block/parallels.c static int parallels_open(BlockDriverState *bs, QDict *options, int flags, · line 714
Quoted 8 lines
block/parallels.c +1
-1@@ -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?
Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/3] block/parallals: Do not update header or truncate image when INMIGRATE
Quoted 19 lines · Stefan Hajnoczi
Quoted 18 lines · Jeff Cody
If we write or modify the image file while the QEMU run state is INMIGRATE, then the
BDRV_O_INACTIVEBDS 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 <[email protected]>--- block/parallels.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)block/parallels.c +0
-0@@ -708,7 +708,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, s->prealloc_mode =PRL_PREALLOC_MODE_FALLOCATE ; }
Quoted 6 lines
block/parallels.c static int parallels_open(BlockDriverState *bs, QDict *options, int flags, · line 714
Quoted 5 lines
block/parallels.c +1
-1-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_MAGICis 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
Quoted 20 lines · Denis V. Lunev
Quoted 19 lines · Stefan Hajnoczi
Quoted 18 lines · Jeff Cody
If we write or modify the image file while the QEMU run state is INMIGRATE, then the
BDRV_O_INACTIVEBDS 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 <[email protected]>--- block/parallels.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)block/parallels.c +0
-0@@ -708,7 +708,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, s->prealloc_mode =PRL_PREALLOC_MODE_FALLOCATE ; }
Quoted 9 lines
Quoted 6 lines
block/parallels.c static int parallels_open(BlockDriverState *bs, QDict *options, int flags, · line 714
Quoted 5 lines
block/parallels.c +1
-1-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_MAGICis set on the destination?good point. it should be set in
invalidate_cachefollowing 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
Quoted 27 lines · Jeff Cody
Quoted 26 lines · Denis V. Lunev
Quoted 25 lines · Stefan Hajnoczi
Quoted 15 lines · Jeff Cody
If we write or modify the image file while the QEMU run state is INMIGRATE, then the
BDRV_O_INACTIVEBDS 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 <[email protected]>--- block/parallels.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)block/parallels.c +0
-0block/parallels.c static int parallels_open(BlockDriverState *bs, QDict *options, int flags, · line 714
Quoted 8 lines
block/parallels.c +1
-1@@ -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_MAGICis set on the destination?
Quoted 7 lines
Quoted 3 lines
good point. it should be set in
invalidate_cachefollowing 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
PATCH v2 3/3 qemu-iotests: update unsupported image formats in 194
In reply to Jeff CodyTest 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).
--- tests/qemu-iotests/194 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
tests/qemu-iotests/194 +1 -1
@@ -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, \
Signature · 1 line
2.13.6
Quoted 12 lines · Jeff Cody
Changes from
v1->v2:* Drop previous parallels patches, just check
BDRV_O_INACTIVEnow (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
Quoted 3 lines
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'
Quoted 17 lines
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 194block/parallels.c | 7 ++----- block/vhdx.c | 7 ------- tests/qemu-iotests/194 | 2 +- 3 files changed, 3 insertions(+), 13 deletions(-)
Signature · 1 line
Alexey
Quoted 13 lines · Jeff Cody
Changes from
v1->v2:* Drop previous parallels patches, just check
BDRV_O_INACTIVEnow (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
Quoted 2 lines
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? :)
Quoted 23 lines
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 194block/parallels.c | 7 ++----- block/vhdx.c | 7 ------- tests/qemu-iotests/194 | 2 +- 3 files changed, 3 insertions(+), 13 deletions(-)Signature · 1 line
2.13.6