diff mbox

[v8,01/16] block: Release dirty bitmaps in bdrv_close()

Message ID 1453917600-2663-2-git-send-email-mreitz@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Max Reitz Jan. 27, 2016, 5:59 p.m. UTC
bdrv_delete() is not very happy about deleting BlockDriverStates with
dirty bitmaps still attached to them. In the past, we got around that
very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and
bdrv_close() simply ignoring that condition. We should fix that by
releasing all dirty bitmaps in bdrv_close() and drop the assertion in
bdrv_delete().

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 block.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

Comments

Fam Zheng Jan. 28, 2016, 3:01 a.m. UTC | #1
On Wed, 01/27 18:59, Max Reitz wrote:
> bdrv_delete() is not very happy about deleting BlockDriverStates with
> dirty bitmaps still attached to them. In the past, we got around that
> very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and
> bdrv_close() simply ignoring that condition. We should fix that by
> releasing all dirty bitmaps in bdrv_close() and drop the assertion in
> bdrv_delete().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> ---
>  block.c | 37 +++++++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 5709d3d..9a31e20 100644
> --- a/block.c
> +++ b/block.c
> @@ -88,6 +88,8 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
>                               const BdrvChildRole *child_role, Error **errp);
>  
>  static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs);
> +
>  /* If non-zero, use only whitelisted block drivers */
>  static int use_bdrv_whitelist;
>  
> @@ -2157,6 +2159,8 @@ void bdrv_close(BlockDriverState *bs)
>  
>      notifier_list_notify(&bs->close_notifiers, bs);
>  
> +    bdrv_release_all_dirty_bitmaps(bs);
> +
>      if (bs->blk) {
>          blk_dev_change_media_cb(bs->blk, false);
>      }
> @@ -2366,7 +2370,6 @@ static void bdrv_delete(BlockDriverState *bs)
>      assert(!bs->job);
>      assert(bdrv_op_blocker_is_empty(bs));
>      assert(!bs->refcnt);
> -    assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>  
>      bdrv_close(bs);
>  
> @@ -3582,21 +3585,39 @@ static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>      }
>  }
>  
> -void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
> +                                                  BdrvDirtyBitmap *bitmap)
>  {
>      BdrvDirtyBitmap *bm, *next;
>      QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
> -        if (bm == bitmap) {
> +        if (!bitmap || bm == bitmap) {
>              assert(!bdrv_dirty_bitmap_frozen(bm));
> -            QLIST_REMOVE(bitmap, list);
> -            hbitmap_free(bitmap->bitmap);
> -            g_free(bitmap->name);
> -            g_free(bitmap);
> -            return;
> +            QLIST_REMOVE(bm, list);
> +            hbitmap_free(bm->bitmap);
> +            g_free(bm->name);
> +            g_free(bm);
> +
> +            if (bitmap) {
> +                return;
> +            }
>          }
>      }
>  }
>  
> +void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +{
> +    bdrv_do_release_matching_dirty_bitmap(bs, bitmap);
> +}
> +
> +/**
> + * Release all dirty bitmaps attached to a BDS (for use in bdrv_close()). There
> + * must not be any frozen bitmaps attached.

Should we assert that? And IIUC the intention of this function is to release
all monitor owned (i.e. user created) dirty bitmaps, which must be named. If
so, can we assert that too?

Fam

> + */
> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs)
> +{
> +    bdrv_do_release_matching_dirty_bitmap(bs, NULL);
> +}
> +
>  void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>  {
>      assert(!bdrv_dirty_bitmap_frozen(bitmap));
> -- 
> 2.7.0
>
Max Reitz Jan. 29, 2016, 1:27 p.m. UTC | #2
On 28.01.2016 04:01, Fam Zheng wrote:
> On Wed, 01/27 18:59, Max Reitz wrote:
>> bdrv_delete() is not very happy about deleting BlockDriverStates with
>> dirty bitmaps still attached to them. In the past, we got around that
>> very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and
>> bdrv_close() simply ignoring that condition. We should fix that by
>> releasing all dirty bitmaps in bdrv_close() and drop the assertion in
>> bdrv_delete().
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>> ---
>>  block.c | 37 +++++++++++++++++++++++++++++--------
>>  1 file changed, 29 insertions(+), 8 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 5709d3d..9a31e20 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -88,6 +88,8 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
>>                               const BdrvChildRole *child_role, Error **errp);
>>  
>>  static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
>> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs);
>> +
>>  /* If non-zero, use only whitelisted block drivers */
>>  static int use_bdrv_whitelist;
>>  
>> @@ -2157,6 +2159,8 @@ void bdrv_close(BlockDriverState *bs)
>>  
>>      notifier_list_notify(&bs->close_notifiers, bs);
>>  
>> +    bdrv_release_all_dirty_bitmaps(bs);
>> +
>>      if (bs->blk) {
>>          blk_dev_change_media_cb(bs->blk, false);
>>      }
>> @@ -2366,7 +2370,6 @@ static void bdrv_delete(BlockDriverState *bs)
>>      assert(!bs->job);
>>      assert(bdrv_op_blocker_is_empty(bs));
>>      assert(!bs->refcnt);
>> -    assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>>  
>>      bdrv_close(bs);
>>  
>> @@ -3582,21 +3585,39 @@ static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>>      }
>>  }
>>  
>> -void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>> +static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
>> +                                                  BdrvDirtyBitmap *bitmap)
>>  {
>>      BdrvDirtyBitmap *bm, *next;
>>      QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
>> -        if (bm == bitmap) {
>> +        if (!bitmap || bm == bitmap) {
>>              assert(!bdrv_dirty_bitmap_frozen(bm));
>> -            QLIST_REMOVE(bitmap, list);
>> -            hbitmap_free(bitmap->bitmap);
>> -            g_free(bitmap->name);
>> -            g_free(bitmap);
>> -            return;
>> +            QLIST_REMOVE(bm, list);
>> +            hbitmap_free(bm->bitmap);
>> +            g_free(bm->name);
>> +            g_free(bm);
>> +
>> +            if (bitmap) {
>> +                return;
>> +            }
>>          }
>>      }
>>  }
>>  
>> +void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>> +{
>> +    bdrv_do_release_matching_dirty_bitmap(bs, bitmap);
>> +}
>> +
>> +/**
>> + * Release all dirty bitmaps attached to a BDS (for use in bdrv_close()). There
>> + * must not be any frozen bitmaps attached.
> 
> Should we assert that?

Well, it is asserted in bdrv_do_release_matching_dirty_bitmap().

>                        And IIUC the intention of this function is to release
> all monitor owned (i.e. user created) dirty bitmaps, which must be named. If
> so, can we assert that too?

Probably we can. I don't really know if it would be worth it, though
(even if it isn't much code). There's not much harm in releasing unnamed
bitmaps here, the main issue would be the question "where did they come
from?".

If I do it, I'll rename the function bdrv_release_named_dirty_bitmaps()
and then assert in bdrv_close() that no bitmaps are attached any more.

Max

> 
> Fam
> 
>> + */
>> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs)
>> +{
>> +    bdrv_do_release_matching_dirty_bitmap(bs, NULL);
>> +}
>> +
>>  void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>  {
>>      assert(!bdrv_dirty_bitmap_frozen(bitmap));
>> -- 
>> 2.7.0
>>
diff mbox

Patch

diff --git a/block.c b/block.c
index 5709d3d..9a31e20 100644
--- a/block.c
+++ b/block.c
@@ -88,6 +88,8 @@  static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
                              const BdrvChildRole *child_role, Error **errp);
 
 static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
+static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs);
+
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
 
@@ -2157,6 +2159,8 @@  void bdrv_close(BlockDriverState *bs)
 
     notifier_list_notify(&bs->close_notifiers, bs);
 
+    bdrv_release_all_dirty_bitmaps(bs);
+
     if (bs->blk) {
         blk_dev_change_media_cb(bs->blk, false);
     }
@@ -2366,7 +2370,6 @@  static void bdrv_delete(BlockDriverState *bs)
     assert(!bs->job);
     assert(bdrv_op_blocker_is_empty(bs));
     assert(!bs->refcnt);
-    assert(QLIST_EMPTY(&bs->dirty_bitmaps));
 
     bdrv_close(bs);
 
@@ -3582,21 +3585,39 @@  static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
     }
 }
 
-void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
+                                                  BdrvDirtyBitmap *bitmap)
 {
     BdrvDirtyBitmap *bm, *next;
     QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
-        if (bm == bitmap) {
+        if (!bitmap || bm == bitmap) {
             assert(!bdrv_dirty_bitmap_frozen(bm));
-            QLIST_REMOVE(bitmap, list);
-            hbitmap_free(bitmap->bitmap);
-            g_free(bitmap->name);
-            g_free(bitmap);
-            return;
+            QLIST_REMOVE(bm, list);
+            hbitmap_free(bm->bitmap);
+            g_free(bm->name);
+            g_free(bm);
+
+            if (bitmap) {
+                return;
+            }
         }
     }
 }
 
+void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+    bdrv_do_release_matching_dirty_bitmap(bs, bitmap);
+}
+
+/**
+ * Release all dirty bitmaps attached to a BDS (for use in bdrv_close()). There
+ * must not be any frozen bitmaps attached.
+ */
+static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs)
+{
+    bdrv_do_release_matching_dirty_bitmap(bs, NULL);
+}
+
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
     assert(!bdrv_dirty_bitmap_frozen(bitmap));