* [Qemu-devel] [PATCH v2 0/3] Don't write headers if BDS is INACTIVE
@ 2017-10-30 13:10 Jeff Cody
2017-10-30 13:10 ` [Qemu-devel] [PATCH v2 1/3] block/vhdx.c: Don't blindly update the header Jeff Cody
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Jeff Cody @ 2017-10-30 13:10 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, kwolf, aik, mreitz, den, stefanha
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v2 1/3] block/vhdx.c: Don't blindly update the header
2017-10-30 13:10 [Qemu-devel] [PATCH v2 0/3] Don't write headers if BDS is INACTIVE Jeff Cody
@ 2017-10-30 13:10 ` Jeff Cody
2017-10-30 13:10 ` [Qemu-devel] [PATCH v2 2/3] block/parallals: Do not update header or truncate image when INMIGRATE Jeff Cody
` (3 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Jeff Cody @ 2017-10-30 13:10 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, kwolf, aik, mreitz, den, stefanha
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v2 2/3] block/parallals: Do not update header or truncate image when INMIGRATE
2017-10-30 13:10 [Qemu-devel] [PATCH v2 0/3] Don't write headers if BDS is INACTIVE Jeff Cody
2017-10-30 13:10 ` [Qemu-devel] [PATCH v2 1/3] block/vhdx.c: Don't blindly update the header Jeff Cody
@ 2017-10-30 13:10 ` Jeff Cody
2017-11-03 11:05 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2017-10-30 13:10 ` [Qemu-devel] [PATCH v2 3/3] qemu-iotests: update unsupported image formats in 194 Jeff Cody
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Jeff Cody @ 2017-10-30 13:10 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, kwolf, aik, mreitz, den, stefanha
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v2 3/3] qemu-iotests: update unsupported image formats in 194
2017-10-30 13:10 [Qemu-devel] [PATCH v2 0/3] Don't write headers if BDS is INACTIVE Jeff Cody
2017-10-30 13:10 ` [Qemu-devel] [PATCH v2 1/3] block/vhdx.c: Don't blindly update the header Jeff Cody
2017-10-30 13:10 ` [Qemu-devel] [PATCH v2 2/3] block/parallals: Do not update header or truncate image when INMIGRATE Jeff Cody
@ 2017-10-30 13:10 ` Jeff Cody
2017-11-02 1:17 ` [Qemu-devel] [PATCH v2 0/3] Don't write headers if BDS is INACTIVE Alexey Kardashevskiy
2017-11-03 10:55 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
4 siblings, 0 replies; 10+ messages in thread
From: Jeff Cody @ 2017-10-30 13:10 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, kwolf, aik, mreitz, den, stefanha
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/3] Don't write headers if BDS is INACTIVE
2017-10-30 13:10 [Qemu-devel] [PATCH v2 0/3] Don't write headers if BDS is INACTIVE Jeff Cody
` (2 preceding siblings ...)
2017-10-30 13:10 ` [Qemu-devel] [PATCH v2 3/3] qemu-iotests: update unsupported image formats in 194 Jeff Cody
@ 2017-11-02 1:17 ` Alexey Kardashevskiy
2017-11-03 10:55 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
4 siblings, 0 replies; 10+ messages in thread
From: Alexey Kardashevskiy @ 2017-11-02 1:17 UTC (permalink / raw)
To: Jeff Cody, qemu-block; +Cc: qemu-devel, kwolf, mreitz, den, stefanha
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] Don't write headers if BDS is INACTIVE
2017-10-30 13:10 [Qemu-devel] [PATCH v2 0/3] Don't write headers if BDS is INACTIVE Jeff Cody
` (3 preceding siblings ...)
2017-11-02 1:17 ` [Qemu-devel] [PATCH v2 0/3] Don't write headers if BDS is INACTIVE Alexey Kardashevskiy
@ 2017-11-03 10:55 ` Stefan Hajnoczi
4 siblings, 0 replies; 10+ messages in thread
From: Stefan Hajnoczi @ 2017-11-03 10:55 UTC (permalink / raw)
To: Jeff Cody; +Cc: qemu-block, kwolf, aik, qemu-devel, mreitz, stefanha, den
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/3] block/parallals: Do not update header or truncate image when INMIGRATE
2017-10-30 13:10 ` [Qemu-devel] [PATCH v2 2/3] block/parallals: Do not update header or truncate image when INMIGRATE Jeff Cody
@ 2017-11-03 11:05 ` Stefan Hajnoczi
2017-11-03 11:08 ` Denis V. Lunev
0 siblings, 1 reply; 10+ messages in thread
From: Stefan Hajnoczi @ 2017-11-03 11:05 UTC (permalink / raw)
To: Jeff Cody; +Cc: qemu-block, kwolf, aik, qemu-devel, mreitz, stefanha, den
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/3] block/parallals: Do not update header or truncate image when INMIGRATE
2017-11-03 11:05 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
@ 2017-11-03 11:08 ` Denis V. Lunev
2017-11-06 20:23 ` Jeff Cody
0 siblings, 1 reply; 10+ messages in thread
From: Denis V. Lunev @ 2017-11-03 11:08 UTC (permalink / raw)
To: Stefan Hajnoczi, Jeff Cody
Cc: qemu-block, kwolf, aik, qemu-devel, mreitz, stefanha
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/3] block/parallals: Do not update header or truncate image when INMIGRATE
2017-11-03 11:08 ` Denis V. Lunev
@ 2017-11-06 20:23 ` Jeff Cody
2017-11-07 2:25 ` Jeff Cody
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Cody @ 2017-11-06 20:23 UTC (permalink / raw)
To: Denis V. Lunev
Cc: Stefan Hajnoczi, qemu-block, kwolf, aik, qemu-devel, mreitz, stefanha
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/3] block/parallals: Do not update header or truncate image when INMIGRATE
2017-11-06 20:23 ` Jeff Cody
@ 2017-11-07 2:25 ` Jeff Cody
0 siblings, 0 replies; 10+ messages in thread
From: Jeff Cody @ 2017-11-07 2:25 UTC (permalink / raw)
To: Denis V. Lunev
Cc: Stefan Hajnoczi, qemu-block, kwolf, aik, qemu-devel, mreitz, stefanha
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-11-07 2:26 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-30 13:10 [Qemu-devel] [PATCH v2 0/3] Don't write headers if BDS is INACTIVE Jeff Cody
2017-10-30 13:10 ` [Qemu-devel] [PATCH v2 1/3] block/vhdx.c: Don't blindly update the header Jeff Cody
2017-10-30 13:10 ` [Qemu-devel] [PATCH v2 2/3] block/parallals: Do not update header or truncate image when INMIGRATE Jeff Cody
2017-11-03 11:05 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2017-11-03 11:08 ` Denis V. Lunev
2017-11-06 20:23 ` Jeff Cody
2017-11-07 2:25 ` Jeff Cody
2017-10-30 13:10 ` [Qemu-devel] [PATCH v2 3/3] qemu-iotests: update unsupported image formats in 194 Jeff Cody
2017-11-02 1:17 ` [Qemu-devel] [PATCH v2 0/3] Don't write headers if BDS is INACTIVE Alexey Kardashevskiy
2017-11-03 10:55 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.