Re: [libvirt] [PATCH v2 5/6] block/dirty-bitmaps: unify qmp_locked and user_locked calls
On 2/18/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
> 14.02.2019 2:23, John Snow wrote:
>> These mean the same thing now. Unify them and rename the merged call
>> bdrv_dirty_bitmap_busy to indicate semantically what we are describing,
>> as well as help disambiguate from the various _locked and _unlocked
>> versions of bitmap helpers that refer to mutex locks.
>>
>> Signed-off-by: John Snow
>> Reviewed-by: Eric Blake
>> ---
>> block/dirty-bitmap.c | 41 +++---
>> blockdev.c | 18 +++
>> include/block/dirty-bitmap.h | 5 ++---
>> migration/block-dirty-bitmap.c | 6 ++---
>> nbd/server.c | 6 ++---
>> 5 files changed, 35 insertions(+), 41 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 2042c62602..8ab048385a 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -48,9 +48,9 @@ struct BdrvDirtyBitmap {
>> QemuMutex *mutex;
>> HBitmap *bitmap;/* Dirty bitmap implementation */
>> HBitmap *meta; /* Meta dirty bitmap */
>> -bool qmp_locked;/* Bitmap is locked, it can't be modified
>> - through QMP */
>> -BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked
>> state */
>> +bool busy; /* Bitmap is busy, it can't be modified
>> through
>> + QMP */
>
> better not "modified" but "used".. for example, export through NBD is not a
> modification.
>
True.
>> +BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
>
> hm this comment change about successor relates more to previous patch, but I
> don't really care.
>
Oh, true.
>> char *name; /* Optional non-empty unique ID */
>> int64_t size; /* Size of the bitmap, in bytes */
>> bool disabled; /* Bitmap is disabled. It ignores all
>> writes to
>> @@ -188,22 +188,17 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap
>> *bitmap)
>> return bitmap->successor;
>> }
>>
>
>
> In comment for bdrv_dirty_bitmap_create_successor, there is "locked" word,
> which you forget to fix to "busy"
> with at least this fixed:
Good spot. Too many occurrences of the word "lock" to have looked for
them all.
> Reviewed-by: Vladimir Sementsov-Ogievskiy
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 5/6] block/dirty-bitmaps: unify qmp_locked and user_locked calls
14.02.2019 2:23, John Snow wrote:
> These mean the same thing now. Unify them and rename the merged call
> bdrv_dirty_bitmap_busy to indicate semantically what we are describing,
> as well as help disambiguate from the various _locked and _unlocked
> versions of bitmap helpers that refer to mutex locks.
>
> Signed-off-by: John Snow
> Reviewed-by: Eric Blake
> ---
> block/dirty-bitmap.c | 41 +++---
> blockdev.c | 18 +++
> include/block/dirty-bitmap.h | 5 ++---
> migration/block-dirty-bitmap.c | 6 ++---
> nbd/server.c | 6 ++---
> 5 files changed, 35 insertions(+), 41 deletions(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 2042c62602..8ab048385a 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -48,9 +48,9 @@ struct BdrvDirtyBitmap {
> QemuMutex *mutex;
> HBitmap *bitmap;/* Dirty bitmap implementation */
> HBitmap *meta; /* Meta dirty bitmap */
> -bool qmp_locked;/* Bitmap is locked, it can't be modified
> - through QMP */
> -BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked
> state */
> +bool busy; /* Bitmap is busy, it can't be modified
> through
> + QMP */
better not "modified" but "used".. for example, export through NBD is not a
modification.
> +BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
hm this comment change about successor relates more to previous patch, but I
don't really care.
> char *name; /* Optional non-empty unique ID */
> int64_t size; /* Size of the bitmap, in bytes */
> bool disabled; /* Bitmap is disabled. It ignores all
> writes to
> @@ -188,22 +188,17 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap
> *bitmap)
> return bitmap->successor;
> }
>
In comment for bdrv_dirty_bitmap_create_successor, there is "locked" word,
which you forget to fix to "busy"
with at least this fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy
--
Best regards,
Vladimir
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 5/6] block/dirty-bitmaps: unify qmp_locked and user_locked calls
These mean the same thing now. Unify them and rename the merged call
bdrv_dirty_bitmap_busy to indicate semantically what we are describing,
as well as help disambiguate from the various _locked and _unlocked
versions of bitmap helpers that refer to mutex locks.
Signed-off-by: John Snow
Reviewed-by: Eric Blake
---
block/dirty-bitmap.c | 41 +++---
blockdev.c | 18 +++
include/block/dirty-bitmap.h | 5 ++---
migration/block-dirty-bitmap.c | 6 ++---
nbd/server.c | 6 ++---
5 files changed, 35 insertions(+), 41 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 2042c62602..8ab048385a 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -48,9 +48,9 @@ struct BdrvDirtyBitmap {
QemuMutex *mutex;
HBitmap *bitmap;/* Dirty bitmap implementation */
HBitmap *meta; /* Meta dirty bitmap */
-bool qmp_locked;/* Bitmap is locked, it can't be modified
- through QMP */
-BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked state
*/
+bool busy; /* Bitmap is busy, it can't be modified through
+ QMP */
+BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
char *name; /* Optional non-empty unique ID */
int64_t size; /* Size of the bitmap, in bytes */
bool disabled; /* Bitmap is disabled. It ignores all writes to
@@ -188,22 +188,17 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap
*bitmap)
return bitmap->successor;
}
-bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
-return bdrv_dirty_bitmap_qmp_locked(bitmap);
+bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
+return bitmap->busy;
}
-void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked)
+void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy)
{
qemu_mutex_lock(bitmap->mutex);
-bitmap->qmp_locked = qmp_locked;
+bitmap->busy = busy;
qemu_mutex_unlock(bitmap->mutex);
}
-bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap)
-{
-return bitmap->qmp_locked;
-}
-
/* Called with BQL taken. */
bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
{
@@ -215,7 +210,7 @@ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap
*bitmap)
{
if (bdrv_dirty_bitmap_has_successor(bitmap)) {
return DIRTY_BITMAP_STATUS_FROZEN;
-} else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
+} else if (bdrv_dirty_bitmap_busy(bitmap)) {
return DIRTY_BITMAP_STATUS_LOCKED;
} else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
return DIRTY_BITMAP_STATUS_DISABLED;
@@ -242,7 +237,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
uint64_t granularity;
BdrvDirtyBitmap *child;
-if (bdrv_dirty_bitmap_user_locked(bitmap)) {
+if (bdrv_dirty_bitmap_busy(bitmap)) {
error_setg(errp, "Cannot create a successor for a bitmap that is
in-use "
"by an operation");
return -1;
@@ -266,7 +261,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
/* Install the successor and lock the parent */
bitmap->successor = child;
-bitmap->qmp_locked = true;
+bitmap->busy = true;
return 0;
}
@@ -288,7 +283,7 @@ void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap
*bitmap)
static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
{
assert(!bitmap->active_iterators);
-assert(!bdrv_dirty_bitmap_user_locked(bitmap));
+assert(!bdrv_dirty_bitmap_busy(bitmap));
assert(!bitmap->meta);
QLIST_REMOVE(bitmap, list);
hbitmap_free(bitmap->bitmap);
@@ -320,7 +315,7 @@ BdrvDirtyBitmap
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
bitmap->successor = NULL;
successor->persistent = bitmap->persistent;
bitmap->persistent = false;
-bitmap->qmp_locked = false;
+bitmap->busy = false;
bdrv_release_dirty_bitmap(bs, bitmap);
return successor;
@@ -329,7 +324,7 @@ BdrvDirtyBitmap
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
/**
* In cases of failure where we can no longer safely delete the parent,
* we may wish to re-join the parent and child/successor.
- * The merged parent will not be user_locked, nor explicitly re-enabled.
+ * The merged parent will not be busy, nor explicitly re-enabled.
* Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.
*/
BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
@@ -349,7 +344,7 @@ BdrvDirtyBitmap
*bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
}
parent->disabled = successor->disabled;
-parent->qmp_locked = false;
+parent->busy = false;
bdrv_release_dirty_bitmap_locked(successor);
parent->successor = NULL;
@@ -380,7 +375,7 @@ void
