Re: [Qemu-block] [PATCH v8 05/20] dirty-bitmap: Avoid size query failure during truncate

[Fam Zheng]

On Mon, 09/18 13:58, Eric Blake wrote:
> We've previously fixed several places where we failed to account
> for possible errors from bdrv_nb_sectors().  Fix another one by
> making bdrv_dirty_bitmap_truncate() take the new size from the
> caller instead of querying itself; then adjust the sole caller
> bdrv_truncate() to pass the size just determined by a successful
> resize, or to skip the bitmap resize on failure, thus avoiding
> sizing the bitmaps to -1.
> 
> Signed-off-by: Eric Blake 
> 
> ---
> v8: retitle and rework to avoid possibility of secondary failure [John]
> v7: new patch [Kevin]
> ---
>  include/block/dirty-bitmap.h |  2 +-
>  block.c  | 15 ++-
>  block/dirty-bitmap.c |  6 +++---
>  3 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 8fd842eac9..7a27590047 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -83,7 +83,7 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
>  void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);
>  int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
>  int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
> -void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);
>  bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>  bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>  bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
> diff --git a/block.c b/block.c
> index ee6a48976e..61ee9d4b83 100644
> --- a/block.c
> +++ b/block.c
> @@ -3450,12 +3450,17 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, 
> PreallocMode prealloc,
>  assert(!(bs->open_flags & BDRV_O_INACTIVE));
> 
>  ret = drv->bdrv_truncate(bs, offset, prealloc, errp);
> -if (ret == 0) {
> -ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
> -bdrv_dirty_bitmap_truncate(bs);
> -bdrv_parent_cb_resize(bs);
> -atomic_inc(>write_gen);
> +if (ret < 0) {
> +return ret;
>  }
> +ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
> +if (ret < 0) {

Ugh, if we get here the situation is a bit embarrassing, because...

> +error_setg_errno(errp, -ret, "Could not refresh total sector count");
> +return ret;
> +}
> +bdrv_dirty_bitmap_truncate(bs, bs->total_sectors * BDRV_SECTOR_SIZE);
> +bdrv_parent_cb_resize(bs);
> +atomic_inc(>write_gen);

I think we still want to inc write_gen even if refresh_total_sectors failed, if
drv->bdrv_truncate has succeeded? That way the next bdrv_co_flush will actually
flush the metadata change to disk.

Maybe similarly call bdrv_parent_cb_resize() as long as drv->bdrv_truncate()
succeeded? The effect is the virtual devices notify guest about this "resized"
event, which I think is correct.

Fam

[Qemu-block] [PATCH v8 05/20] dirty-bitmap: Avoid size query failure during truncate

[Eric Blake]

We've previously fixed several places where we failed to account
for possible errors from bdrv_nb_sectors().  Fix another one by
making bdrv_dirty_bitmap_truncate() take the new size from the
caller instead of querying itself; then adjust the sole caller
bdrv_truncate() to pass the size just determined by a successful
resize, or to skip the bitmap resize on failure, thus avoiding
sizing the bitmaps to -1.

Signed-off-by: Eric Blake 

---
v8: retitle and rework to avoid possibility of secondary failure [John]
v7: new patch [Kevin]
---
 include/block/dirty-bitmap.h |  2 +-
 block.c  | 15 ++-
 block/dirty-bitmap.c |  6 +++---
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 8fd842eac9..7a27590047 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -83,7 +83,7 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
 void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);
 int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
 int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
-void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
+void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);
 bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
 bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
 bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
diff --git a/block.c b/block.c
index ee6a48976e..61ee9d4b83 100644
--- a/block.c
+++ b/block.c
@@ -3450,12 +3450,17 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, 
PreallocMode prealloc,
 assert(!(bs->open_flags & BDRV_O_INACTIVE));

 ret = drv->bdrv_truncate(bs, offset, prealloc, errp);
-if (ret == 0) {
-ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
-bdrv_dirty_bitmap_truncate(bs);
-bdrv_parent_cb_resize(bs);
-atomic_inc(>write_gen);
+if (ret < 0) {
+return ret;
 }
+ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Could not refresh total sector count");
+return ret;
+}
+bdrv_dirty_bitmap_truncate(bs, bs->total_sectors * BDRV_SECTOR_SIZE);
+bdrv_parent_cb_resize(bs);
+atomic_inc(>write_gen);
 return ret;
 }

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 42a55e4a4b..ee164fb518 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -1,7 +1,7 @@
 /*
  * Block Dirty Bitmap
  *
- * Copyright (c) 2016 Red Hat. Inc
+ * Copyright (c) 2016-2017 Red Hat. Inc
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to 
deal
@@ -302,10 +302,10 @@ BdrvDirtyBitmap 
*bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
  * Truncates _all_ bitmaps attached to a BDS.
  * Called with BQL taken.
  */
-void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
+void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes)
 {
 BdrvDirtyBitmap *bitmap;
-uint64_t size = bdrv_nb_sectors(bs);
+int64_t size = DIV_ROUND_UP(bytes, BDRV_SECTOR_SIZE);

 bdrv_dirty_bitmaps_lock(bs);
 QLIST_FOREACH(bitmap, >dirty_bitmaps, list) {
-- 
2.13.5