Re: [Qemu-devel] [PATCH 7/8] block: Pass truncate exact=true where reasonable

[Maxim Levitsky]

On Wed, 2019-09-18 at 11:51 +0200, Max Reitz wrote:
> This is a change in behavior, so all instances need a good
> justification.  The comments added here should explain my reasoning.
> 
> qed already had a comment that suggests it always expected
> bdrv_truncate()/blk_truncate() to behave as if exact=true were passed
> (c743849bee7 came eight months before 55b949c8476), so it was simply
> broken until now.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block/parallels.c | 11 +++++++++--
>  block/qcow2.c     |  6 +++++-
>  block/qed.c       |  2 +-
>  qemu-img.c        |  7 ++++++-
>  qemu-io-cmds.c    |  7 ++++++-
>  5 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index a1a92c97a4..603211f83c 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -487,7 +487,12 @@ static int coroutine_fn 
> parallels_co_check(BlockDriverState *bs,
>          res->leaks += count;
>          if (fix & BDRV_FIX_LEAKS) {
>              Error *local_err = NULL;
> -            ret = bdrv_truncate(bs->file, res->image_end_offset, false,
> +
> +            /*
> +             * In order to really repair the image, we must shrink it.
> +             * That means we have to pass exact=true.
> +             */
> +            ret = bdrv_truncate(bs->file, res->image_end_offset, true,
>                                  PREALLOC_MODE_OFF, &local_err);

I am not familiar with the parallels format, but in theory,
a partial fix is better that nothing.

>              if (ret < 0) {
>                  error_report_err(local_err);
> @@ -880,7 +885,9 @@ static void parallels_close(BlockDriverState *bs)
>      if ((bs->open_flags & BDRV_O_RDWR) && !(bs->open_flags & 
> BDRV_O_INACTIVE)) {
>          s->header->inuse = 0;
>          parallels_update_header(bs);
> -        bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, false,
> +
> +        /* errors are ignored, so we might as well pass exact=true */
> +        bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true,
>                        PREALLOC_MODE_OFF, NULL);
Fair enough.

>      }
>  
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 4ef19dd29a..eba165de7f 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -5057,7 +5057,11 @@ static int qcow2_amend_options(BlockDriverState *bs, 
> QemuOpts *opts,
>              return ret;
>          }
>  
> -        ret = blk_truncate(blk, new_size, false, PREALLOC_MODE_OFF, errp);
> +        /*
> +         * Amending image options should ensure that the image has
> +         * exactly the given new values, so pass exact=true here.
> +         */
Agree.

> +        ret = blk_truncate(blk, new_size, true, PREALLOC_MODE_OFF, errp);
>          blk_unref(blk);
>          if (ret < 0) {
>              return ret;
> diff --git a/block/qed.c b/block/qed.c
> index 7c2a65af40..8005cfc305 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -674,7 +674,7 @@ static int coroutine_fn 
> bdrv_qed_co_create(BlockdevCreateOptions *opts,
>      l1_size = header.cluster_size * header.table_size;
>  
>      /* File must start empty and grow, check truncate is supported */
I would update the above comment, with something like

"QED format requires the underlying file to have the exact expected length,
which is 0 on creation"
Or something similar.

> -    ret = blk_truncate(blk, 0, false, PREALLOC_MODE_OFF, errp);
> +    ret = blk_truncate(blk, 0, true, PREALLOC_MODE_OFF, errp);
>      if (ret < 0) {
>          goto out;
>      }
> diff --git a/qemu-img.c b/qemu-img.c
> index f8694f4f72..a3169b6113 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3823,7 +3823,12 @@ static int img_resize(int argc, char **argv)
>          }
>      }
>  
> -    ret = blk_truncate(blk, total_size, false, prealloc, &err);
> +    /*
> +     * The user expects the image to have the desired size after
> +     * resizing, so pass @exact=true.  It is of no use to report
> +     * success when the image has not actually been resized.
> +     */
Agree.

> +    ret = blk_truncate(blk, total_size, true, prealloc, &err);
>      if (ret < 0) {
>          error_report_err(err);
>          goto out;
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 5e9017c979..1b7e700020 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -1710,7 +1710,12 @@ static int truncate_f(BlockBackend *blk, int argc, 
> char **argv)
>          return offset;
>      }
>  
> -    ret = blk_truncate(blk, offset, false, PREALLOC_MODE_OFF, &local_err);
> +    /*
> +     * qemu-io is a debugging tool, so let us be strict here and pass
> +     * exact=true.  It is better to err on the "emit more errors" side
> +     * than to be overly permissive.
> +     */
Also agree.

> +    ret = blk_truncate(blk, offset, true, PREALLOC_MODE_OFF, &local_err);
>      if (ret < 0) {
>          error_report_err(local_err);
>          return ret;


Other than few nitpicks which probably aren't important,
Reviewed-by: Maxim Levitsky <address@hidden>

Best regards,
        Maxim Levitsky