Re: [libvirt] [PATCH v3 07/10] block/dirty-bitmaps: unify qmp_locked and user_locked calls
On 2/25/19 7:03 AM, Vladimir Sementsov-Ogievskiy wrote: > 23.02.2019 3:06, 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 > > Hmm, and here, you directly fixed what I've noticed, so my r-b, > of course, still applies. > Sorry for not adding them. I didn't plan to merge the series until you got a chance to review it, so I wasn't worried about missing it in the long run. Sorry if it makes it harder to see which ones you'd like to look at. > Ha, but I noticed funny thing: > >> --- >> block/dirty-bitmap.c | 40 +++--- >> blockdev.c | 18 +++ >> include/block/dirty-bitmap.h | 5 ++--- >> migration/block-dirty-bitmap.c | 6 ++--- >> nbd/server.c | 6 ++--- >> 5 files changed, 34 insertions(+), 41 deletions(-) >> >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index d92a269753..b3627b0d8c 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c > > [..] > >> @@ -265,9 +259,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState >> *bs, >> child->disabled = bitmap->disabled; >> bitmap->disabled = true; >> >> -/* Install the successor and lock the parent */ >> +/* Install the successor and mark the parent as busy */ > > I asked you to fix comment for bdrv_dirty_bitmap_create_successor(), but I > didn't > noticed this one, I meant the comment to the whole function, placed above it, > which > now have > "Requires that the bitmap is not user_locked and has no successor." > > So, it should be updated too. > > and with it: > Reviewed-by: Vladimir Sementsov-Ogievskiy > Ha. OK, updated to "is not marked busy" Thanks for the reviews! >> bitmap->successor = child; >> -bitmap->qmp_locked = true; >> +bitmap->busy = true; >> return 0; >> } >> > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 07/10] block/dirty-bitmaps: unify qmp_locked and user_locked calls
23.02.2019 3:06, 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 Hmm, and here, you directly fixed what I've noticed, so my r-b, of course, still applies. Ha, but I noticed funny thing: > --- > block/dirty-bitmap.c | 40 +++--- > blockdev.c | 18 +++ > include/block/dirty-bitmap.h | 5 ++--- > migration/block-dirty-bitmap.c | 6 ++--- > nbd/server.c | 6 ++--- > 5 files changed, 34 insertions(+), 41 deletions(-) > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index d92a269753..b3627b0d8c 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c [..] > @@ -265,9 +259,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState > *bs, > child->disabled = bitmap->disabled; > bitmap->disabled = true; > > -/* Install the successor and lock the parent */ > +/* Install the successor and mark the parent as busy */ I asked you to fix comment for bdrv_dirty_bitmap_create_successor(), but I didn't noticed this one, I meant the comment to the whole function, placed above it, which now have "Requires that the bitmap is not user_locked and has no successor." So, it should be updated too. and with it: Reviewed-by: Vladimir Sementsov-Ogievskiy > bitmap->successor = child; > -bitmap->qmp_locked = true; > +bitmap->busy = true; > return 0; > } > -- Best regards, Vladimir -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 07/10] block/dirty-bitmaps: unify qmp_locked and user_locked calls
On 2/22/19 6:06 PM, 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 -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 07/10] 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
---
block/dirty-bitmap.c | 40 +++---
blockdev.c | 18 +++
include/block/dirty-bitmap.h | 5 ++---
migration/block-dirty-bitmap.c | 6 ++---
nbd/server.c | 6 ++---
5 files changed, 34 insertions(+), 41 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index d92a269753..b3627b0d8c 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -48,8 +48,7 @@ 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 */
+bool busy; /* Bitmap is busy, it can't be used via QMP */
BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
char *name; /* Optional non-empty unique ID */
int64_t size; /* Size of the bitmap, in bytes */
@@ -188,22 +187,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 +209,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;
@@ -243,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;
@@ -265,9 +259,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
child->disabled = bitmap->disabled;
bitmap->disabled = true;
-/* Install the successor and lock the parent */
+/* Install the successor and mark the parent as busy */
bitmap->successor = child;
-bitmap->qmp_locked = true;
+bitmap->busy = true;
return 0;
}
@@ -289,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);
@@ -321,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;
@@ -330,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.
+ * The merged parent will be marked as not busy.
* The marged parent will be enabled if and only if the successor was enabled.
* Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.
*/
@@ -351,7 +345,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;
@@ -382,7 +376,7 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs,
int64_t bytes)
bdrv_dirty_bitmaps_lock(bs);
QLIST_FOREACH(bitmap,
