[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 1/9] block: add bdrv_measure() API
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v5 1/9] block: add bdrv_measure() API |
Date: |
Wed, 19 Apr 2017 14:27:37 +0100 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
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 <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.
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.
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH v5 2/9] raw-format: add bdrv_measure() support, Stefan Hajnoczi, 2017/04/18
[Qemu-devel] [PATCH v5 4/9] qcow2: make refcount size calculation conservative, Stefan Hajnoczi, 2017/04/18
[Qemu-devel] [PATCH v5 3/9] qcow2: extract preallocation calculation function, Stefan Hajnoczi, 2017/04/18
[Qemu-devel] [PATCH v5 5/9] qcow2: extract image creation option parsing, Stefan Hajnoczi, 2017/04/18
[Qemu-devel] [PATCH v5 6/9] qcow2: add bdrv_measure() support, Stefan Hajnoczi, 2017/04/18
[Qemu-devel] [PATCH v5 7/9] qemu-img: add measure subcommand, Stefan Hajnoczi, 2017/04/18
[Qemu-devel] [PATCH v5 8/9] qemu-iotests: support per-format golden output files, Stefan Hajnoczi, 2017/04/18
[Qemu-devel] [PATCH v5 9/9] iotests: add test 178 for qemu-img measure, Stefan Hajnoczi, 2017/04/18