[1/2] parallels: create parallels_inactivate() callback of block driver state
diff mbox

Message ID 1491405126-30591-2-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev April 5, 2017, 3:12 p.m. UTC
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(-)

Comments

Stefan Hajnoczi April 6, 2017, 3:14 p.m. UTC | #1
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?
Denis V. Lunev April 6, 2017, 3:22 p.m. UTC | #2
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 :)

Patch
diff mbox

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,