| Message ID | 20200217150246.29180-8-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix error handling during bitmap postcopy | expand |
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 <vsementsov@virtuozzo.com> > --- > 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(&s->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, &error_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, &error_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(&s->lock); > } > > Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
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 <vsementsov@virtuozzo.com> >> --- >> 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(&s->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, &error_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, &error_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(&s->lock); >> } >> > > Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
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 <vsementsov@virtuozzo.com> >>> --- >>> 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(&s->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, &error_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, &error_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(&s->lock); >>> } >>> >> >> Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > >
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(&s->lock); + if (bdrv_dirty_bitmap_has_successor(s->bitmap)) { + bdrv_reclaim_dirty_bitmap(s->bitmap, &error_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, &error_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(&s->lock); }
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 <vsementsov@virtuozzo.com> --- migration/block-dirty-bitmap.c | 25 ++++--------------------- 1 file changed, 4 insertions(+), 21 deletions(-)