Re: [PATCH v2 07/22] migration/block-dirty-bitmap: simplify dirty_bitmap_load_complete

2020-02-19 Thread Vladimir Sementsov-Ogievskiy 

19.02.2020 18:30, Vladimir Sementsov-Ogievskiy wrote:

18.02.2020 17:26, Andrey Shinkevich wrote:

On 17/02/2020 18:02, Vladimir Sementsov-Ogievskiy wrote:

bdrv_enable_dirty_bitmap_locked() call does nothing, as if we are in
postcopy, bitmap successor must be enabled, and reclaim operation will
enable the bitmap.

So, actually we need just call _reclaim_ in both if branches, and
making differences only to add an assertion seems not really good. The
logic becomes simple: on load complete we do reclaim and that's all.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  migration/block-dirty-bitmap.c | 25 -
  1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 440c41cfca..9cc750d93b 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -535,6 +535,10 @@ static void dirty_bitmap_load_complete(QEMUFile *f, 
DBMLoadState *s)
  qemu_mutex_lock(>lock);
+    if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {

What about making it sure?
    assert(!s->bitmap->successor->disabled);


I'm afraid we can't as BdrvDirtyBitmap is not public structure




+    bdrv_reclaim_dirty_bitmap(s->bitmap, _abort);


But we can assert that resulting bitmap is enabled.


Or not, as bitmap may be not yet enabled, if guest is not yet started.




+    }
+
  for (item = s->enabled_bitmaps; item; item = g_slist_next(item)) {
  LoadBitmapState *b = item->data;
@@ -544,27 +548,6 @@ static void dirty_bitmap_load_complete(QEMUFile *f, 
DBMLoadState *s)
  }
  }
-    if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {
-    bdrv_dirty_bitmap_lock(s->bitmap);
-    if (s->enabled_bitmaps == NULL) {
-    /* in postcopy */
-    bdrv_reclaim_dirty_bitmap_locked(s->bitmap, _abort);
-    bdrv_enable_dirty_bitmap_locked(s->bitmap);
-    } else {
-    /* target not started, successor must be empty */
-    int64_t count = bdrv_get_dirty_count(s->bitmap);
-    BdrvDirtyBitmap *ret = bdrv_reclaim_dirty_bitmap_locked(s->bitmap,
-    NULL);
-    /* bdrv_reclaim_dirty_bitmap can fail only on no successor (it
- * must be) or on merge fail, but merge can't fail when second
- * bitmap is empty
- */
-    assert(ret == s->bitmap &&
-   count == bdrv_get_dirty_count(s->bitmap));
-    }
-    bdrv_dirty_bitmap_unlock(s->bitmap);
-    }
-
  qemu_mutex_unlock(>lock);
  }



Reviewed-by: Andrey Shinkevich 






--
Best regards,
Vladimir

Re: [PATCH v2 07/22] migration/block-dirty-bitmap: simplify dirty_bitmap_load_complete

2020-02-19 Thread Vladimir Sementsov-Ogievskiy 

18.02.2020 17:26, Andrey Shinkevich wrote:

On 17/02/2020 18:02, Vladimir Sementsov-Ogievskiy wrote:

bdrv_enable_dirty_bitmap_locked() call does nothing, as if we are in
postcopy, bitmap successor must be enabled, and reclaim operation will
enable the bitmap.

So, actually we need just call _reclaim_ in both if branches, and
making differences only to add an assertion seems not really good. The
logic becomes simple: on load complete we do reclaim and that's all.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  migration/block-dirty-bitmap.c | 25 -
  1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 440c41cfca..9cc750d93b 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -535,6 +535,10 @@ static void dirty_bitmap_load_complete(QEMUFile *f, 
DBMLoadState *s)
  qemu_mutex_lock(>lock);
+    if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {

What about making it sure?
    assert(!s->bitmap->successor->disabled);


I'm afraid we can't as BdrvDirtyBitmap is not public structure




+    bdrv_reclaim_dirty_bitmap(s->bitmap, _abort);


But we can assert that resulting bitmap is enabled.


+    }
+
  for (item = s->enabled_bitmaps; item; item = g_slist_next(item)) {
  LoadBitmapState *b = item->data;
@@ -544,27 +548,6 @@ static void dirty_bitmap_load_complete(QEMUFile *f, 
DBMLoadState *s)
  }
  }
-    if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {
-    bdrv_dirty_bitmap_lock(s->bitmap);
-    if (s->enabled_bitmaps == NULL) {
-    /* in postcopy */
-    bdrv_reclaim_dirty_bitmap_locked(s->bitmap, _abort);
-    bdrv_enable_dirty_bitmap_locked(s->bitmap);
-    } else {
-    /* target not started, successor must be empty */
-    int64_t count = bdrv_get_dirty_count(s->bitmap);
-    BdrvDirtyBitmap *ret = bdrv_reclaim_dirty_bitmap_locked(s->bitmap,
-    NULL);
-    /* bdrv_reclaim_dirty_bitmap can fail only on no successor (it
- * must be) or on merge fail, but merge can't fail when second
- * bitmap is empty
- */
-    assert(ret == s->bitmap &&
-   count == bdrv_get_dirty_count(s->bitmap));
-    }
-    bdrv_dirty_bitmap_unlock(s->bitmap);
-    }
-
  qemu_mutex_unlock(>lock);
  }



Reviewed-by: Andrey Shinkevich 



--
Best regards,
Vladimir

Re: [PATCH v2 07/22] migration/block-dirty-bitmap: simplify dirty_bitmap_load_complete

2020-02-18 Thread Andrey Shinkevich 

On 17/02/2020 18:02, Vladimir Sementsov-Ogievskiy wrote:

bdrv_enable_dirty_bitmap_locked() call does nothing, as if we are in
postcopy, bitmap successor must be enabled, and reclaim operation will
enable the bitmap.

So, actually we need just call _reclaim_ in both if branches, and
making differences only to add an assertion seems not really good. The
logic becomes simple: on load complete we do reclaim and that's all.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  migration/block-dirty-bitmap.c | 25 -
  1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 440c41cfca..9cc750d93b 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -535,6 +535,10 @@ static void dirty_bitmap_load_complete(QEMUFile *f, 
DBMLoadState *s)
  
  qemu_mutex_lock(>lock);
  
+if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {

What about making it sure?
   assert(!s->bitmap->successor->disabled);


+bdrv_reclaim_dirty_bitmap(s->bitmap, _abort);
+}
+
  for (item = s->enabled_bitmaps; item; item = g_slist_next(item)) {
  LoadBitmapState *b = item->data;
  
@@ -544,27 +548,6 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DBMLoadState *s)

  }
  }
  
-if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {

-bdrv_dirty_bitmap_lock(s->bitmap);
-if (s->enabled_bitmaps == NULL) {
-/* in postcopy */
-bdrv_reclaim_dirty_bitmap_locked(s->bitmap, _abort);
-bdrv_enable_dirty_bitmap_locked(s->bitmap);
-} else {
-/* target not started, successor must be empty */
-int64_t count = bdrv_get_dirty_count(s->bitmap);
-BdrvDirtyBitmap *ret = bdrv_reclaim_dirty_bitmap_locked(s->bitmap,
-NULL);
-/* bdrv_reclaim_dirty_bitmap can fail only on no successor (it
- * must be) or on merge fail, but merge can't fail when second
- * bitmap is empty
- */
-assert(ret == s->bitmap &&
-   count == bdrv_get_dirty_count(s->bitmap));
-}
-bdrv_dirty_bitmap_unlock(s->bitmap);
-}
-
  qemu_mutex_unlock(>lock);
  }
  



Reviewed-by: Andrey Shinkevich 
--
With the best regards,
Andrey Shinkevich

[PATCH v2 07/22] migration/block-dirty-bitmap: simplify dirty_bitmap_load_complete

2020-02-17 Thread Vladimir Sementsov-Ogievskiy 
bdrv_enable_dirty_bitmap_locked() call does nothing, as if we are in
postcopy, bitmap successor must be enabled, and reclaim operation will
enable the bitmap.

So, actually we need just call _reclaim_ in both if branches, and
making differences only to add an assertion seems not really good. The
logic becomes simple: on load complete we do reclaim and that's all.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 migration/block-dirty-bitmap.c | 25 -
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 440c41cfca..9cc750d93b 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -535,6 +535,10 @@ static void dirty_bitmap_load_complete(QEMUFile *f, 
DBMLoadState *s)
 
 qemu_mutex_lock(>lock);
 
+if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {
+bdrv_reclaim_dirty_bitmap(s->bitmap, _abort);
+}
+
 for (item = s->enabled_bitmaps; item; item = g_slist_next(item)) {
 LoadBitmapState *b = item->data;
 
@@ -544,27 +548,6 @@ static void dirty_bitmap_load_complete(QEMUFile *f, 
DBMLoadState *s)
 }
 }
 
-if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {
-bdrv_dirty_bitmap_lock(s->bitmap);
-if (s->enabled_bitmaps == NULL) {
-/* in postcopy */
-bdrv_reclaim_dirty_bitmap_locked(s->bitmap, _abort);
-bdrv_enable_dirty_bitmap_locked(s->bitmap);
-} else {
-/* target not started, successor must be empty */
-int64_t count = bdrv_get_dirty_count(s->bitmap);
-BdrvDirtyBitmap *ret = bdrv_reclaim_dirty_bitmap_locked(s->bitmap,
-NULL);
-/* bdrv_reclaim_dirty_bitmap can fail only on no successor (it
- * must be) or on merge fail, but merge can't fail when second
- * bitmap is empty
- */
-assert(ret == s->bitmap &&
-   count == bdrv_get_dirty_count(s->bitmap));
-}
-bdrv_dirty_bitmap_unlock(s->bitmap);
-}
-
 qemu_mutex_unlock(>lock);
 }
 
-- 
2.21.0