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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4676 bytes --]

On Tue, Apr 18, 2017 at 10:08:20AM -0500, Eric Blake wrote:
> 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 <stefanha@redhat.com>
> > Reviewed-by: Alberto Garcia <berto@igalia.com>
> > ---
> >  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.

Thanks for the feedback.  I'll clarify what "size" means.

> > +#
> > +# 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().

Yes, that makes the function more self-explanatory.

> > +++ 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?

.bdrv_measure() is a 1:1 pass-through of the public bdrv_measure()
function.  All the public function does is to dereference
drv->bdrv_measure.

I think that's why many of the other callbacks also have no
documentation - they inherit semantics from the public function.  We
don't need to duplicate the doc comments.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]