Re: [Qemu-block] [Qemu-devel] [PATCH v7 05/20] dirty-bitmap: Check for size query failure during truncate

[John Snow]

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 <address@hidden>
> 
> ---
> 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?