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() report the error rather then
silently resizing bitmaps to -1.  Then adjust the sole caller
bdrv_truncate() to both reduce the likelihood of failure (blindly
calling bdrv_dirty_bitmap_truncate() after refresh_total_sectors()
fails was not nice) as well as propagate any actual failures.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v7: new patch [Kevin]
---
 include/block/dirty-bitmap.h |  2 +-
 block.c                      | 19 ++++++++++++++-----
 block/dirty-bitmap.c         | 12 ++++++++----
 3 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 8fd842eac9..15101b59d5 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);
+int bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
 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..790dcce360 100644
--- a/block.c
+++ b/block.c
@@ -3450,12 +3450,21 @@ 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(&bs->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;
+    }
+    ret = bdrv_dirty_bitmap_truncate(bs);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not refresh total sector count");
+        return ret;
+    }
+    bdrv_parent_cb_resize(bs);
+    atomic_inc(&bs->write_gen);
     return ret;
 }

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 42a55e4a4b..52f7a399b2 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
@@ -300,13 +300,16 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,

 /**
  * Truncates _all_ bitmaps attached to a BDS.
- * Called with BQL taken.
+ * Called with BQL taken, returns -errno on failure.
  */
-void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
+int bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
 {
     BdrvDirtyBitmap *bitmap;
-    uint64_t size = bdrv_nb_sectors(bs);
+    int64_t size = bdrv_nb_sectors(bs);

+    if (size < 0) {
+        return size;
+    }
     bdrv_dirty_bitmaps_lock(bs);
     QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
         assert(!bdrv_dirty_bitmap_frozen(bitmap));
@@ -315,6 +318,7 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
         bitmap->size = size;
     }
     bdrv_dirty_bitmaps_unlock(bs);
+    return 0;
 }

 static bool bdrv_dirty_bitmap_has_name(BdrvDirtyBitmap *bitmap)
-- 
2.13.5

On 09/12/2017 04:31 PM, 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() report the error rather then
> silently resizing bitmaps to -1.  Then adjust the sole caller

Nice failure mode. It was not immediately obvious to me that this could
fail, but here we all are.

> bdrv_truncate() to both reduce the likelihood of failure (blindly
> calling bdrv_dirty_bitmap_truncate() after refresh_total_sectors()
> fails was not nice) as well as propagate any actual failures.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v7: new patch [Kevin]
> ---
>  include/block/dirty-bitmap.h |  2 +-
>  block.c                      | 19 ++++++++++++++-----
>  block/dirty-bitmap.c         | 12 ++++++++----
>  3 files changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 8fd842eac9..15101b59d5 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);
> +int bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
>  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..790dcce360 100644
> --- a/block.c
> +++ b/block.c
> @@ -3450,12 +3450,21 @@ 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(&bs->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;
> +    }
> +    ret = bdrv_dirty_bitmap_truncate(bs);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Could not refresh total sector count");

You probably meant to write a different error message here.

Also, what happens if the actual truncate call works, but the bitmap
resizing fails? What state does that leave us in?

On 09/13/2017 06:27 PM, John Snow wrote:
> 
> 
> On 09/12/2017 04:31 PM, 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() report the error rather then
>> silently resizing bitmaps to -1.  Then adjust the sole caller
> 
> Nice failure mode. It was not immediately obvious to me that this could
> fail, but here we all are.
> 
>> bdrv_truncate() to both reduce the likelihood of failure (blindly
>> calling bdrv_dirty_bitmap_truncate() after refresh_total_sectors()
>> fails was not nice) as well as propagate any actual failures.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>

>>      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(&bs->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;
>> +    }
>> +    ret = bdrv_dirty_bitmap_truncate(bs);
>> +    if (ret < 0) {
>> +        error_setg_errno(errp, -ret, "Could not refresh total sector count");
> 
> You probably meant to write a different error message here.
> 
> Also, what happens if the actual truncate call works, but the bitmap
> resizing fails? What state does that leave us in?

Another option: bdrv_dirty_bitmap_truncate() can only fail if
bdrv_nb_sectors() can fail.  We WANT to use the actual size of the
device (which might be slightly different than the requested size passed
to bdrv_truncate in the first place), but we could change
bdrv_dirty_bitmap_truncate() to take an actual size as a parameter
instead of having to query bdrv_nb_sectors() for the size; and we can
change the contract of refresh_total_sectors() to query the actual size
before returning (remember, offset >> BDRV_SECTOR_BITS is only the hint
size, and may differ from the actual size).  That way, if
refresh_total_sectors() succeeds, then bdrv_dirty_bitmap_truncate()
cannot fail.

I'm not sure, however, how invasive it will be to make
refresh_total_sectors() guarantee that it can return the actual size
that was set even when that size differs from the hint.  Still, it
sounds like the right approach to take, so I'll play with it.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org