[Qemu-devel] [PATCH 0/2] parallels: some truncate fixes

[Qemu-devel] [PATCH 0/2] parallels: some truncate fixes

[Denis V. Lunev]

Stefan, here some obvious fixes for parallels image format. I am not quite
sure that they should go into 2.9, but it will be good if they will reach
2.9.1

Your opinion is very welcome here.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Stefan Hajnoczi <stefanha@redhat.com>

Denis V. Lunev (2):
  parallels: create parallels_inactivate() callback of block driver
    state
  parallels: relax check for auto switch of prealloc_mode

 block/parallels.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] parallels: create parallels_inactivate() callback of block driver state

[Denis V. Lunev]

We should avoid to image file at migration end when BDRV_O_INACTIVE
is set on the block driver state. All modifications should be done
in advance, i.e. in bdrv_inactivate.

The patch also adds final bdrv_flush() to be sure that changes have
reached disk finally before other side will access the image.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 90acf79..9dea09e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -728,18 +728,35 @@ fail_options:
     goto fail;
 }
 
-
-static void parallels_close(BlockDriverState *bs)
+static int parallels_inactivate(BlockDriverState *bs)
 {
+    int err;
     BDRVParallelsState *s = bs->opaque;
 
-    if (bs->open_flags & BDRV_O_RDWR) {
-        s->header->inuse = 0;
-        parallels_update_header(bs);
+    if (!(bs->open_flags & BDRV_O_RDWR)) {
+        return 0;
+    }
+
+    s->header->inuse = 0;
+    err = parallels_update_header(bs);
+    if (err < 0) {
+        return err;
+    }
+
+    err = bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS);
+    if (err < 0) {
+        return err;
     }
 
-    if (bs->open_flags & BDRV_O_RDWR) {
-        bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS);
+    return bdrv_flush(bs->file->bs);
+}
+
+static void parallels_close(BlockDriverState *bs)
+{
+    BDRVParallelsState *s = bs->opaque;
+
+    if (!(bs->open_flags & BDRV_O_INACTIVE)) {
+        parallels_inactivate(bs);
     }
 
     g_free(s->bat_dirty_bmap);
@@ -777,6 +794,7 @@ static BlockDriver bdrv_parallels = {
     .bdrv_co_flush_to_os      = parallels_co_flush_to_os,
     .bdrv_co_readv  = parallels_co_readv,
     .bdrv_co_writev = parallels_co_writev,
+    .bdrv_inactivate = parallels_inactivate,
 
     .bdrv_create    = parallels_create,
     .bdrv_check     = parallels_check,
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] parallels: relax check for auto switch of prealloc_mode

[Denis V. Lunev]

PRL_PREALLOC_MODE_TRUNCATE can be set only through command line. Remove
the check that bdrv_truncate() is working on, this is almost always the
case, while the check could lead to serious consequences during migration
etc when we should not even try to API which technically could change the
image (asserts are possible).

Let us keep this simple.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 9dea09e..e805034 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -695,8 +695,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail_options;
     }
 
-    if (!(flags & BDRV_O_RESIZE) || !bdrv_has_zero_init(bs->file->bs) ||
-            bdrv_truncate(bs->file, bdrv_getlength(bs->file->bs)) != 0) {
+    if (!bdrv_has_zero_init(bs->file->bs) != 0) {
         s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
     }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] parallels: create parallels_inactivate() callback of block driver state

[Stefan Hajnoczi]

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

On Wed, Apr 05, 2017 at 06:12:05PM +0300, Denis V. Lunev wrote:
> We should avoid to image file at migration end when BDRV_O_INACTIVE

s/avoid to image/avoid writing to the image/ ?

> is set on the block driver state. All modifications should be done
> in advance, i.e. in bdrv_inactivate.
> 
> The patch also adds final bdrv_flush() to be sure that changes have
> reached disk finally before other side will access the image.

If migration fails/cancels on the source after .bdrv_inactivate() we
need to return to the "activated" state.  .bdrv_invalidate_cache() will
be called so I think it needs to be implemented by parallels to set
s->header->inuse again if BDRV_O_RDWR.

.bdrv_invalidate_cache() is also called on the destination at live
migration handover.  That's okay - it makes sense for the destination to
set the inuse field on success.

> -static void parallels_close(BlockDriverState *bs)
> +static int parallels_inactivate(BlockDriverState *bs)
>  {
> +    int err;
>      BDRVParallelsState *s = bs->opaque;
>  
> -    if (bs->open_flags & BDRV_O_RDWR) {
> -        s->header->inuse = 0;
> -        parallels_update_header(bs);
> +    if (!(bs->open_flags & BDRV_O_RDWR)) {
> +        return 0;
> +    }
> +
> +    s->header->inuse = 0;
> +    err = parallels_update_header(bs);
> +    if (err < 0) {

Should we set s->header->inuse again in all error paths in case
something calls parallels_hupdate_header() again later?

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

Re: [Qemu-devel] [PATCH 1/2] parallels: create parallels_inactivate() callback of block driver state

[Denis V. Lunev]

On 04/06/2017 06:14 PM, Stefan Hajnoczi wrote:
> On Wed, Apr 05, 2017 at 06:12:05PM +0300, Denis V. Lunev wrote:
>> We should avoid to image file at migration end when BDRV_O_INACTIVE
> s/avoid to image/avoid writing to the image/ ?
yes


>> is set on the block driver state. All modifications should be done
>> in advance, i.e. in bdrv_inactivate.
>>
>> The patch also adds final bdrv_flush() to be sure that changes have
>> reached disk finally before other side will access the image.
> If migration fails/cancels on the source after .bdrv_inactivate() we
> need to return to the "activated" state.  .bdrv_invalidate_cache() will
> be called so I think it needs to be implemented by parallels to set
> s->header->inuse again if BDRV_O_RDWR.
>
> .bdrv_invalidate_cache() is also called on the destination at live
> migration handover.  That's okay - it makes sense for the destination to
> set the inuse field on success.
hmm. sounds like a good catch..


>> -static void parallels_close(BlockDriverState *bs)
>> +static int parallels_inactivate(BlockDriverState *bs)
>>  {
>> +    int err;
>>      BDRVParallelsState *s = bs->opaque;
>>  
>> -    if (bs->open_flags & BDRV_O_RDWR) {
>> -        s->header->inuse = 0;
>> -        parallels_update_header(bs);
>> +    if (!(bs->open_flags & BDRV_O_RDWR)) {
>> +        return 0;
>> +    }
>> +
>> +    s->header->inuse = 0;
>> +    err = parallels_update_header(bs);
>> +    if (err < 0) {
> Should we set s->header->inuse again in all error paths in case
> something calls parallels_hupdate_header() again later?
yes.

thank you :)

Re: [Qemu-devel] [PATCH 2/2] parallels: relax check for auto switch of prealloc_mode

[Stefan Hajnoczi]

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

On Wed, Apr 05, 2017 at 06:12:06PM +0300, Denis V. Lunev wrote:
> PRL_PREALLOC_MODE_TRUNCATE can be set only through command line. Remove
> the check that bdrv_truncate() is working on, this is almost always the
> case, while the check could lead to serious consequences during migration
> etc when we should not even try to API which technically could change the
> image (asserts are possible).
> 
> Let us keep this simple.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

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

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