Message ID | 20190213232356.21034-5-jsnow@redhat.com |
---|---|
State | New |
Headers | show |
Series | dirty-bitmaps: deprecate @status field | expand |
14.02.2019 2:23, John Snow wrote: > Instead of implying a locked status, make it explicit. locked interferes with bitmap mutex, so may be better "qmp_locked state", but not sure. > Now, bitmaps in use by migration, NBD or backup operations > are all treated the same way with the same code paths. > > Signed-off-by: John Snow <jsnow@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Hmm. Isn't it better to make successor-related staff unrelated to locking at all? So, backup will call set_qmp_locked like others? And then do create_successor, abdicate, reclaim, whatever it wants, and finally set_qmp_locked(false) ? To make it work even more in the same path. But it may be done separately, if we want. > --- > block/dirty-bitmap.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index bb2e19e0d8..2042c62602 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -188,10 +188,8 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap) > return bitmap->successor; > } > > -/* Both conditions disallow user-modification via QMP. */ > bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) { > - return bdrv_dirty_bitmap_has_successor(bitmap) || > - bdrv_dirty_bitmap_qmp_locked(bitmap); > + return bdrv_dirty_bitmap_qmp_locked(bitmap); > } > > void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked) > @@ -266,8 +264,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, > child->disabled = bitmap->disabled; > bitmap->disabled = true; > > - /* Install the successor and freeze the parent */ > + /* Install the successor and lock the parent */ > bitmap->successor = child; > + bitmap->qmp_locked = true; > return 0; > } > > @@ -321,6 +320,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, > bitmap->successor = NULL; > successor->persistent = bitmap->persistent; > bitmap->persistent = false; > + bitmap->qmp_locked = false; > bdrv_release_dirty_bitmap(bs, bitmap); > > return successor; > @@ -349,6 +349,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, > } > > parent->disabled = successor->disabled; > + parent->qmp_locked = false; > bdrv_release_dirty_bitmap_locked(successor); > parent->successor = NULL; > >
On 2/18/19 11:52 AM, Vladimir Sementsov-Ogievskiy wrote: > 14.02.2019 2:23, John Snow wrote: >> Instead of implying a locked status, make it explicit. > > locked interferes with bitmap mutex, so may be better "qmp_locked state", but not sure. > I agree that "locked" has too many meanings, so in patch 5 I start using the term "busy" instead. >> Now, bitmaps in use by migration, NBD or backup operations >> are all treated the same way with the same code paths. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > Hmm. Isn't it better to make successor-related staff unrelated to locking at all? Maybe -- but it doesn't make sense to allow users to modify bitmaps that have a successor because we know it's definitely busy. I'll take a further cleanup patch if you think it's better -- just be careful to make sure that any interface calls will work gracefully with a bitmap with a successor. > So, backup will call set_qmp_locked like others? And then do create_successor, > abdicate, reclaim, whatever it wants, and finally set_qmp_locked(false) ? > To make it work even more in the same path. But it may be done separately, if we > want. >
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index bb2e19e0d8..2042c62602 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -188,10 +188,8 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap) return bitmap->successor; } -/* Both conditions disallow user-modification via QMP. */ bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) { - return bdrv_dirty_bitmap_has_successor(bitmap) || - bdrv_dirty_bitmap_qmp_locked(bitmap); + return bdrv_dirty_bitmap_qmp_locked(bitmap); } void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked) @@ -266,8 +264,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, child->disabled = bitmap->disabled; bitmap->disabled = true; - /* Install the successor and freeze the parent */ + /* Install the successor and lock the parent */ bitmap->successor = child; + bitmap->qmp_locked = true; return 0; } @@ -321,6 +320,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, bitmap->successor = NULL; successor->persistent = bitmap->persistent; bitmap->persistent = false; + bitmap->qmp_locked = false; bdrv_release_dirty_bitmap(bs, bitmap); return successor; @@ -349,6 +349,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, } parent->disabled = successor->disabled; + parent->qmp_locked = false; bdrv_release_dirty_bitmap_locked(successor); parent->successor = NULL;