Re: [Qemu-devel] [PATCH v5 1/9] block: add bdrv_measure() API

[Eric Blake]

On 04/18/2017 08:57 AM, Stefan Hajnoczi wrote:
> bdrv_measure() provides a conservative maximum for the size of a new
> image.  This information is handy if storage needs to be allocated (e.g.
> a SAN or an LVM volume) ahead of time.
> 
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> Reviewed-by: Alberto Garcia <address@hidden>
> ---
>  qapi/block-core.json      | 25 +++++++++++++++++++++++++
>  include/block/block.h     |  4 ++++
>  include/block/block_int.h |  2 ++
>  block.c                   | 35 +++++++++++++++++++++++++++++++++++
>  4 files changed, 66 insertions(+)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 033457c..1ea5536 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -463,6 +463,31 @@
>             '*dirty-bitmaps': ['BlockDirtyInfo'] } }
>  
>  ##
> +# @BlockMeasureInfo:
> +#
> +# Image size calculation information.  This structure describes the size
> +# requirements for creating a new image file.
> +#
> +# The size requirements depend on the new image file format.  File size 
> always
> +# equals virtual disk size for the 'raw' format.  Compact formats such as
> +# 'qcow2' represent unallocated and zero regions efficiently so file size may
> +# be smaller than virtual disk size.

I guess that implies that holes due to a file system that supports them
is NOT considered a compact format under qemu's control.  Or maybe
another way of wording it is that we are reporting the size of all guest
contents that are associated with a host offset by this layer (all
sectors of a raw format have a host offset, even if that offset falls in
the hole of a sparse file; but sectors of qcow2 do not necessarily have
a host offset if they are unallocated or zero).

But I'm not sure my alternative wording adds anything, so I'm also fine
if you don't feel like tweaking it any further.


> +#
> +# The values are upper bounds that are guaranteed to fit the new image file.
> +# Subsequent modification, such as internal snapshot or bitmap creation, may
> +# require additional space and is not covered here.
> +#
> +# @required: Size required for a new image file, in bytes.
> +#
> +# @fully-allocated: Image file size, in bytes, once data has been written
> +#                   to all sectors.
> +#
> +# Since: 2.10
> +##
> +{ 'struct': 'BlockMeasureInfo',
> +  'data': {'required': 'int', 'fully-allocated': 'int'} }
> +

> +++ b/include/block/block.h
> @@ -298,6 +298,10 @@ int bdrv_truncate(BdrvChild *child, int64_t offset);
>  int64_t bdrv_nb_sectors(BlockDriverState *bs);
>  int64_t bdrv_getlength(BlockDriverState *bs);
>  int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
> +void bdrv_measure(BlockDriver *drv, QemuOpts *opts,
> +                  BlockDriverState *in_bs,
> +                  BlockMeasureInfo *info,
> +                  Error **errp);

Would it be any better to have BlockMeasureInfo* be the return value (or
NULL on error), instead of an output-only parameter?  Of course, that
changes the allocation scheme (as written, a caller can stack-allocate
or malloc the pointer it passes in, but with a return value of a
pointer, it will always be malloc'd); on the other hand, the allocation
scheme may matter down the road if the struct ever gains a non-scalar
member where stack-allocation becomes harder to clean up than just
calling qapi_free_BlockMeasureInfo().


> +++ b/include/block/block_int.h
> @@ -201,6 +201,8 @@ struct BlockDriver {
>      int64_t (*bdrv_getlength)(BlockDriverState *bs);
>      bool has_variable_length;
>      int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
> +    void (*bdrv_measure)(QemuOpts *opts, BlockDriverState *in_bs,
> +                         BlockMeasureInfo *info, Error **errp);

I know we haven't done a good job in the past, but should we start
trying to do better at documenting callback constraints of new things
added in this header?

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

signature.asc
Description: OpenPGP digital signature