qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate
Date: Tue, 03 Mar 2015 16:27:51 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0

On 2015-03-03 at 16:24, John Snow wrote:


On 03/03/2015 11:02 AM, Max Reitz wrote:
On 2015-03-02 at 18:20, John Snow wrote:
Signed-off-by: John Snow <address@hidden>
---
  block.c                | 22 ++++++++++++++++++++
  include/block/block.h  |  1 +
  include/qemu/hbitmap.h | 10 +++++++++
  util/hbitmap.c         | 55
++++++++++++++++++++++++++++++++++++++++++++++++++
  4 files changed, 88 insertions(+)

diff --git a/block.c b/block.c
index e6b2696..5eaa874 100644
--- a/block.c
+++ b/block.c
@@ -3543,6 +3543,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t
offset)
      ret = drv->bdrv_truncate(bs, offset);
      if (ret == 0) {
          ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
+        bdrv_dirty_bitmap_truncate(bs);
          if (bs->blk) {
              blk_dev_resize_cb(bs->blk);
          }
@@ -5562,6 +5563,27 @@ BdrvDirtyBitmap
*bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
      return parent;
  }
+static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t
size)
+{
+    /* Should only be frozen during a block backup job, which should
have
+     * blocked any resize actions. */
+    assert(!bdrv_dirty_bitmap_frozen(bitmap));
+    hbitmap_truncate(bitmap->bitmap, size);
+}
+
+void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
+{
+    BdrvDirtyBitmap *bitmap;
+    uint64_t size = bdrv_nb_sectors(bs);
+
+    QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+        if (bdrv_dirty_bitmap_frozen(bitmap)) {
+            continue;
+        }

Hm!

The assert() above doesn't do anything because this condition will just
skip over frozen bitmaps. Maybe we should drop it?

I guess my R-b stands since bdrv_dirty_bitmap_frozen(bitmap) will always
be false anyway (hence the assertion), but it would stand with this
block dropped, too.

Max


It's there for parity with the other bdrv helpers that perform a similar check. I am guarding against, in the future, any potential users from deciding to call this function directly for whichever reason.

Requesting an action on a drive that contains bitmaps that are not ready for the action is OK. Requesting an action on a bitmap that is not ready for the action should never happen, hence the 'documentation assert.'

My default action will be to leave it in.

Technically that's fine (which is why I said my R-b stands), my only problem with that is that if something really strange happens and we actually end up with a frozen bitmap here, the action we're taking is wrong. The bitmap will have the wrong size after the BDS was truncated, and that's not right.

I'd find just calling dirty_bitmap_truncate() regardless of the frozen state more correct, the idea being "We want to resize all bitmaps, because all bitmaps attached to this BDS need to be resized", and then, in dirty_bitmap_truncate(), we notice that we cannot resize a frozen bitmap. But we need to. So the assertion fails.

Max

--js




reply via email to

[Prev in Thread] Current Thread [Next in Thread]