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

[John Snow]

On 09/25/2017 10:55 AM, 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.  This also fixes a bug where not all
> failure paths in bdrv_truncate() would set errp.
> 
> Note that bdrv_truncate() is still a bit awkward.  We may want
> to revisit it later and clean up things to better guarantee that
> a resize attempt either fails cleanly up front, or cannot fail
> after guest-visible changes have been made (if temporary changes
> are made, then they need to be cleanly rolled back).  But that
> is a task for another day; for now, the goal is the bare minimum
> fix to ensure that just bdrv_dirty_bitmap_truncate() cannot fail.
> 
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> v10: always resize bitmap as before [John], enhance commit message to
> point out errp bugfix [Vladimir]
> v9: skip only bdrv_dirty_bitmap_truncate on error [Fam]
> v8: retitle and rework to avoid possibility of secondary failure [John]
> v7: new patch [Kevin]
> ---
>  include/block/dirty-bitmap.h |  2 +-
>  block.c                      | 16 +++++++++++-----
>  block/dirty-bitmap.c         |  6 +++---
>  3 files changed, 15 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 528cda7b2c..ef5af81f66 100644
> --- a/block.c
> +++ b/block.c
> @@ -3545,12 +3545,18 @@ 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");
> +    } else {
> +        offset = bs->total_sectors * BDRV_SECTOR_SIZE;
> +    }
> +    bdrv_dirty_bitmap_truncate(bs, offset);
> +    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..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, &bs->dirty_bitmaps, list) {
> 

I *THINK* this is the most correct we can do for now...

Reviewed-by: John Snow <address@hidden>