Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface

[John Snow]

On 07/12/2017 11:18 AM, Vladimir Sementsov-Ogievskiy wrote:
> 30.06.2017 03:27, John Snow wrote:
> > On 06/06/2017 12:26 PM, Vladimir Sementsov-Ogievskiy wrote:
> > > The function should collect statistics, about used/unused by top-level
> > > format driver space (in its .file) and allocation status
> > > (data/zero/discarded/after-eof) of corresponding areas in this .file.
> > >
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > > ---
> > >   block.c                   | 16 ++++++++++++++
> > >   include/block/block.h     |  3 +++
> > >   include/block/block_int.h |  2 ++
> > >   qapi/block-core.json      | 55 
> > > +++++++++++++++++++++++++++++++++++++++++++++++
> > >   4 files changed, 76 insertions(+)
> > >
> > > diff --git a/block.c b/block.c
> > > index 50ba264143..7d720ae0c2 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -3407,6 +3407,22 @@ int64_t 
> > > bdrv_get_allocated_file_size(BlockDriverState *bs)
> > >   }
> > >   /**
> > > + * Collect format allocation info. See BlockFormatAllocInfo 
> > > definition in
> > > + * qapi/block-core.json.
> > > + */
> > > +int bdrv_get_format_alloc_stat(BlockDriverState *bs, 
> > > BlockFormatAllocInfo *bfai)
> > > +{
> > > +    BlockDriver *drv = bs->drv;
> > > +    if (!drv) {
> > > +        return -ENOMEDIUM;
> > > +    }
> > > +    if (drv->bdrv_get_format_alloc_stat) {
> > > +        return drv->bdrv_get_format_alloc_stat(bs, bfai);
> > > +    }
> > > +    return -ENOTSUP;
> > > +}
> > > +
> > > +/**
> > >    * Return number of sectors on success, -errno on error.
> > >    */
> > >   int64_t bdrv_nb_sectors(BlockDriverState *bs)
> > > diff --git a/include/block/block.h b/include/block/block.h
> > > index 9b355e92d8..646376a772 100644
> > > --- a/include/block/block.h
> > > +++ b/include/block/block.h
> > > @@ -335,6 +335,9 @@ typedef enum {
> > >   int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, 
> > > BdrvCheckMode fix);
> > > +int bdrv_get_format_alloc_stat(BlockDriverState *bs,
> > > +                               BlockFormatAllocInfo *bfai);
> > > +
> > >   /* The units of offset and total_work_size may be chosen 
> > > arbitrarily by the
> > >    * block driver; total_work_size may change during the course of 
> > > the amendment
> > >    * operation */
> > > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > > index 8d3724cce6..458c715e99 100644
> > > --- a/include/block/block_int.h
> > > +++ b/include/block/block_int.h
> > > @@ -208,6 +208,8 @@ struct BlockDriver {
> > >       int64_t (*bdrv_getlength)(BlockDriverState *bs);
> > >       bool has_variable_length;
> > >       int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
> > > +    int (*bdrv_get_format_alloc_stat)(BlockDriverState *bs,
> > > +                                      BlockFormatAllocInfo *bfai);
> > >       int coroutine_fn (*bdrv_co_pwritev_compressed)(BlockDriverState 
> > > *bs,
> > >           uint64_t offset, uint64_t bytes, QEMUIOVector *qiov);
> > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > index ea0b3e8b13..fd7b52bd69 100644
> > > --- a/qapi/block-core.json
> > > +++ b/qapi/block-core.json
> > > @@ -139,6 +139,61 @@
> > >              '*format-specific': 'ImageInfoSpecific' } }
> > >   ##
> > > +# @BlockFormatAllocInfo:
> > > +#
> > > +#
> > > +# Allocation relations between format file and underlying protocol 
> > > file.
> > > +# All fields are in bytes.
> > > +#
> > I guess this is a relation in the sense that the format differentiates
> > between used-unused and the protocol differentiates between
> > data-zero-trim which gives us the 2D matrix, showing a relation between
> > "two" files.
> >
> > I find the use of the word "file" here a bit of a misdirection, though,
> > as qcow2 (or any other format) in this sense is not a file but rather a
> > schema used for interpreting data, and the file itself belongs solely to
> > the protocol.
> >
> > I might suggest phrasing this as ...
> >
> > "Allocation information of an underlying protocol file, as partitioned
> > by a format driver's utilization of said allocations."
> >
> > Maybe that's too wordy. Eric?
> >
> > > +# There are two types of the format file portions: 'used' and 
> > > 'unused'. It's up
> > > +# to the format how to interpret these types. For now the only 
> > > format supporting
> > "format file" seems misleading for similar reasons to my objection
> > above, which is that the format doesn't have a file, exactly. It's an
> > abstract schema.
> >
> > "It's up to the format how to interpret these types" is also a bit too
> > vague to help inform readers what the types mean, IMO.
> >
> > Here's an attempt:
> >
> > "Allocations may be considered either used or unused by the format
> > driver interpreting those allocations. It is at the discretion of the
> > format driver (e.g. qcow2) which regions of its backing storage are
> > considered in-use or not."
>
> So we are saying about "allocations". But unallocated data may be 
> used/unused too, so,
> can we call unallocated areas "allocations"?

You're right, "allocation" may imply something that is extant, but these 
are more like regions ...

Uh;

"Regions" may be considered either used or unused?

or

"Regions of the underlying protocol file may be considered used or 
unused by the format driver interpreting these regions."

Something along these lines, perhaps?