[libvirt] [PATCH v2 4/6] block/dirty-bitmap: explicitly lock bitmaps with successors

[Vladimir Sementsov-Ogievskiy]

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 at redhat.com>
> Reviewed-by: Eric Blake <eblake at redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov at 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;
>   
> 


-- 
Best regards,
Vladimir