1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] block: simplify code around releasing bitmaps

Paolo Bonzini posted 1 patch 6 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180323164254.26487-2-pbonzini@redhat.com
Test checkpatch passed
Test docker-build@min-glib passed
Test docker-mingw@fedora passed
Test docker-quick@centos6 failed
Test s390x passed
There is a newer version of this series
block/dirty-bitmap.c | 81 +++++++++++++++++++---------------------------------
1 file changed, 30 insertions(+), 51 deletions(-)
[Qemu-devel] [PATCH] block: simplify code around releasing bitmaps
Posted by Paolo Bonzini 6 years, 1 month ago 
QLIST_REMOVE does not require walking the list, and once the "bitmap"
argument is removed from bdrv_do_release_matching_dirty_bitmap_locked
the code simplifies a lot and it is worth inlining everything in the
callers of bdrv_do_release_matching_dirty_bitmap.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/dirty-bitmap.c | 81 +++++++++++++++++++---------------------------------
 1 file changed, 30 insertions(+), 51 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 13bb5066a4..c0fb49bf7b 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -250,48 +250,15 @@ void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap)
 }
 
 /* Called within bdrv_dirty_bitmap_lock..unlock */
-static void bdrv_do_release_matching_dirty_bitmap_locked(
-    BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
-    bool (*cond)(BdrvDirtyBitmap *bitmap))
+static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
 {
-    BdrvDirtyBitmap *bm, *next;
-
-    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
-        if ((!bitmap || bm == bitmap) && (!cond || cond(bm))) {
-            assert(!bm->active_iterators);
-            assert(!bdrv_dirty_bitmap_frozen(bm));
-            assert(!bm->meta);
-            QLIST_REMOVE(bm, list);
-            hbitmap_free(bm->bitmap);
-            g_free(bm->name);
-            g_free(bm);
-
-            if (bitmap) {
-                return;
-            }
-        }
-    }
-
-    if (bitmap) {
-        abort();
-    }
-}
-
-/* Called with BQL taken.  */
-static void bdrv_do_release_matching_dirty_bitmap(
-    BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
-    bool (*cond)(BdrvDirtyBitmap *bitmap))
-{
-    bdrv_dirty_bitmaps_lock(bs);
-    bdrv_do_release_matching_dirty_bitmap_locked(bs, bitmap, cond);
-    bdrv_dirty_bitmaps_unlock(bs);
-}
-
-/* Called within bdrv_dirty_bitmap_lock..unlock */
-static void bdrv_release_dirty_bitmap_locked(BlockDriverState *bs,
-                                             BdrvDirtyBitmap *bitmap)
-{
-    bdrv_do_release_matching_dirty_bitmap_locked(bs, bitmap, NULL);
+    assert(!bitmap->active_iterators);
+    assert(!bdrv_dirty_bitmap_frozen(bitmap));
+    assert(!bitmap->meta);
+    QLIST_REMOVE(bitmap, list);
+    hbitmap_free(bitmap->bitmap);
+    g_free(bitmap->name);
+    g_free(bitmap);
 }
 
 /**
@@ -344,7 +311,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
         error_setg(errp, "Merging of parent and successor bitmap failed");
         return NULL;
     }
-    bdrv_release_dirty_bitmap_locked(bs, successor);
+    bdrv_release_dirty_bitmap_locked(successor);
     parent->successor = NULL;
 
     return parent;
@@ -382,15 +349,12 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes)
     bdrv_dirty_bitmaps_unlock(bs);
 }
 
-static bool bdrv_dirty_bitmap_has_name(BdrvDirtyBitmap *bitmap)
-{
-    return !!bdrv_dirty_bitmap_name(bitmap);
-}
-
 /* Called with BQL taken.  */
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
 {
-    bdrv_do_release_matching_dirty_bitmap(bs, bitmap, NULL);
+    bdrv_dirty_bitmaps_lock(bs);
+    bdrv_release_dirty_bitmap_locked(bitmap);
+    bdrv_dirty_bitmaps_unlock(bs);
 }
 
 /**
@@ -401,7 +365,15 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
  */
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
 {
-    bdrv_do_release_matching_dirty_bitmap(bs, NULL, bdrv_dirty_bitmap_has_name);
+    BdrvDirtyBitmap *bm, *next;
+
+    bdrv_dirty_bitmaps_lock(bs);
+    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
+        if (bdrv_dirty_bitmap_name(bm)) {
+            bdrv_release_dirty_bitmap_locked(bm);
+        }
+    }
+    bdrv_dirty_bitmaps_unlock(bs);
 }
 
 /**
@@ -412,8 +384,15 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
  */
 void bdrv_release_persistent_dirty_bitmaps(BlockDriverState *bs)
 {
-    bdrv_do_release_matching_dirty_bitmap(bs, NULL,
-                                          bdrv_dirty_bitmap_get_persistance);
+    BdrvDirtyBitmap *bm, *next;
+
+    bdrv_dirty_bitmaps_lock(bs);
+    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
+        if (bdrv_dirty_bitmap_get_persistance(bm)) {
+            bdrv_release_dirty_bitmap_locked(bm);
+        }
+    }
+    bdrv_dirty_bitmaps_unlock(bs);
 }
 
 /**
-- 
2.16.2
Re: [Qemu-devel] [PATCH] block: simplify code around releasing bitmaps
Posted by Vladimir Sementsov-Ogievskiy 6 years, 1 month ago 
23.03.2018 19:42, Paolo Bonzini wrote:
> QLIST_REMOVE does not require walking the list, and once the "bitmap"
> argument is removed from bdrv_do_release_matching_dirty_bitmap_locked
> the code simplifies a lot and it is worth inlining everything in the
> callers of bdrv_do_release_matching_dirty_bitmap.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block/dirty-bitmap.c | 81 +++++++++++++++++++---------------------------------
>   1 file changed, 30 insertions(+), 51 deletions(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 13bb5066a4..c0fb49bf7b 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -250,48 +250,15 @@ void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap)
>   }
>   
>   /* Called within bdrv_dirty_bitmap_lock..unlock */

Worth adding "Called with BQL taken" to the comment?

> -static void bdrv_do_release_matching_dirty_bitmap_locked(
> -    BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> -    bool (*cond)(BdrvDirtyBitmap *bitmap))
> +static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
>   {
> -    BdrvDirtyBitmap *bm, *next;
> -
> -    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
> -        if ((!bitmap || bm == bitmap) && (!cond || cond(bm))) {
> -            assert(!bm->active_iterators);
> -            assert(!bdrv_dirty_bitmap_frozen(bm));
> -            assert(!bm->meta);
> -            QLIST_REMOVE(bm, list);
> -            hbitmap_free(bm->bitmap);
> -            g_free(bm->name);
> -            g_free(bm);
> -
> -            if (bitmap) {
> -                return;
> -            }
> -        }
> -    }
> -
> -    if (bitmap) {
> -        abort();
> -    }
> -}
> -
> -/* Called with BQL taken.  */
> -static void bdrv_do_release_matching_dirty_bitmap(
> -    BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> -    bool (*cond)(BdrvDirtyBitmap *bitmap))
> -{
> -    bdrv_dirty_bitmaps_lock(bs);
> -    bdrv_do_release_matching_dirty_bitmap_locked(bs, bitmap, cond);
> -    bdrv_dirty_bitmaps_unlock(bs);
> -}
> -
> -/* Called within bdrv_dirty_bitmap_lock..unlock */
> -static void bdrv_release_dirty_bitmap_locked(BlockDriverState *bs,
> -                                             BdrvDirtyBitmap *bitmap)
> -{
> -    bdrv_do_release_matching_dirty_bitmap_locked(bs, bitmap, NULL);
> +    assert(!bitmap->active_iterators);
> +    assert(!bdrv_dirty_bitmap_frozen(bitmap));
> +    assert(!bitmap->meta);
> +    QLIST_REMOVE(bitmap, list);
> +    hbitmap_free(bitmap->bitmap);
> +    g_free(bitmap->name);
> +    g_free(bitmap);
>   }
>   
>   /**
> @@ -344,7 +311,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
>           error_setg(errp, "Merging of parent and successor bitmap failed");
>           return NULL;
>       }
> -    bdrv_release_dirty_bitmap_locked(bs, successor);
> +    bdrv_release_dirty_bitmap_locked(successor);
>       parent->successor = NULL;
>   
>       return parent;
> @@ -382,15 +349,12 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes)
>       bdrv_dirty_bitmaps_unlock(bs);
>   }
>   
> -static bool bdrv_dirty_bitmap_has_name(BdrvDirtyBitmap *bitmap)
> -{
> -    return !!bdrv_dirty_bitmap_name(bitmap);
> -}
> -
>   /* Called with BQL taken.  */
>   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>   {
> -    bdrv_do_release_matching_dirty_bitmap(bs, bitmap, NULL);
> +    bdrv_dirty_bitmaps_lock(bs);
> +    bdrv_release_dirty_bitmap_locked(bitmap);
> +    bdrv_dirty_bitmaps_unlock(bs);
>   }
>   
>   /**
> @@ -401,7 +365,15 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>    */
>   void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
>   {
> -    bdrv_do_release_matching_dirty_bitmap(bs, NULL, bdrv_dirty_bitmap_has_name);
> +    BdrvDirtyBitmap *bm, *next;
> +
> +    bdrv_dirty_bitmaps_lock(bs);
> +    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
> +        if (bdrv_dirty_bitmap_name(bm)) {
> +            bdrv_release_dirty_bitmap_locked(bm);
> +        }
> +    }
> +    bdrv_dirty_bitmaps_unlock(bs);
>   }
>   
>   /**
> @@ -412,8 +384,15 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
>    */

worth adding "Called with BQL taken." to the comment? 
bdrv_release_named_dirty_bitmaps functions has it.

>   void bdrv_release_persistent_dirty_bitmaps(BlockDriverState *bs)
>   {
> -    bdrv_do_release_matching_dirty_bitmap(bs, NULL,
> -                                          bdrv_dirty_bitmap_get_persistance);
> +    BdrvDirtyBitmap *bm, *next;
> +
> +    bdrv_dirty_bitmaps_lock(bs);
> +    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
> +        if (bdrv_dirty_bitmap_get_persistance(bm)) {
> +            bdrv_release_dirty_bitmap_locked(bm);
> +        }
> +    }
> +    bdrv_dirty_bitmaps_unlock(bs);
>   }
>   
>   /**

anyway,

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir
Re: [Qemu-devel] [PATCH] block: simplify code around releasing bitmaps
Posted by Paolo Bonzini 6 years, 1 month ago 
On 26/03/2018 12:24, Vladimir Sementsov-Ogievskiy wrote:
> 23.03.2018 19:42, Paolo Bonzini wrote:
>> QLIST_REMOVE does not require walking the list, and once the "bitmap"
>> argument is removed from bdrv_do_release_matching_dirty_bitmap_locked
>> the code simplifies a lot and it is worth inlining everything in the
>> callers of bdrv_do_release_matching_dirty_bitmap.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   block/dirty-bitmap.c | 81
>> +++++++++++++++++++---------------------------------
>>   1 file changed, 30 insertions(+), 51 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 13bb5066a4..c0fb49bf7b 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -250,48 +250,15 @@ void
>> bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap)
>>   }
>>     /* Called within bdrv_dirty_bitmap_lock..unlock */
> 
> Worth adding "Called with BQL taken" to the comment?

Yes, good idea.

>> +static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)

> worth adding "Called with BQL taken." to the comment?
> bdrv_release_named_dirty_bitmaps functions has it.
> 
>>   void bdrv_release_persistent_dirty_bitmaps(BlockDriverState *bs)

This is pre-existing, but it's also a good idea.

Paolo

>> -    bdrv_do_release_matching_dirty_bitmap(bs, NULL,
>> -                                         
>> bdrv_dirty_bitmap_get_persistance);
>> +    BdrvDirtyBitmap *bm, *next;
>> +
>> +    bdrv_dirty_bitmaps_lock(bs);
>> +    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
>> +        if (bdrv_dirty_bitmap_get_persistance(bm)) {
>> +            bdrv_release_dirty_bitmap_locked(bm);
>> +        }
>> +    }
>> +    bdrv_dirty_bitmaps_unlock(bs);
>>   }
>>     /**
> 
> anyway,
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.