Re: [PATCH v7 25/47] block: Def. impl.s for get_allocated_file_size

[Kevin Wolf]

Am 25.06.2020 um 17:21 hat Max Reitz geschrieben:
> If every BlockDriver were to implement bdrv_get_allocated_file_size(),
> there are basically three ways it would be handled:
> (1) For protocol drivers: Figure out the actual allocated file size in
>     some protocol-specific way
> (2) For protocol drivers: If that is not possible (or we just have not
>     bothered to implement it yet), return -ENOTSUP
> (3) For drivers with children: Return the sum of some or all their
>     children's sizes
> 
> For the drivers we have, case (3) boils down to either:
> (a) The sum of all children's sizes
> (b) The size of the primary child
> 
> (2), (3a) and (3b) can be implemented generically, so this patch adds
> such generic implementations for drivers to use.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/block/block_int.h |  5 ++++
>  block.c                   | 51 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 5da793bfc3..c963ee9f28 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1318,6 +1318,11 @@ int coroutine_fn 
> bdrv_co_block_status_from_backing(BlockDriverState *bs,
>                                                     int64_t *pnum,
>                                                     int64_t *map,
>                                                     BlockDriverState **file);
> +
> +int64_t bdrv_sum_allocated_file_size(BlockDriverState *bs);
> +int64_t bdrv_primary_allocated_file_size(BlockDriverState *bs);
> +int64_t bdrv_notsup_allocated_file_size(BlockDriverState *bs);
> +
>  const char *bdrv_get_parent_name(const BlockDriverState *bs);
>  void blk_dev_change_media_cb(BlockBackend *blk, bool load, Error **errp);
>  bool blk_dev_has_removable_media(BlockBackend *blk);
> diff --git a/block.c b/block.c
> index 1c71ecab7c..fc01ce90b3 100644
> --- a/block.c
> +++ b/block.c
> @@ -5003,6 +5003,57 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState 
> *bs)
>      return -ENOTSUP;
>  }
>  
> +/**
> + * Implementation of BlockDriver.bdrv_get_allocated_file_size() for
> + * block drivers that want it to sum all children they store data on.
> + * (This excludes backing children.)
> + */
> +int64_t bdrv_sum_allocated_file_size(BlockDriverState *bs)
> +{
> +    BdrvChild *child;
> +    int64_t child_size, sum = 0;
> +
> +    QLIST_FOREACH(child, &bs->children, next) {
> +        if (child->role & (BDRV_CHILD_DATA | BDRV_CHILD_METADATA |
> +                           BDRV_CHILD_FILTERED))
> +        {
> +            child_size = bdrv_get_allocated_file_size(child->bs);
> +            if (child_size < 0) {
> +                return child_size;
> +            }
> +            sum += child_size;
> +        }
> +    }
> +
> +    return sum;
> +}

The only user apart from bdrv_get_allocated_file_size() is blkverify. As
I argued that blkverify shouldn't use it, this can become static.

> +/**
> + * Implementation of BlockDriver.bdrv_get_allocated_file_size() for
> + * block drivers that want it to return only the size of a node's
> + * primary child.
> + */
> +int64_t bdrv_primary_allocated_file_size(BlockDriverState *bs)
> +{
> +    BlockDriverState *primary_bs;
> +
> +    primary_bs = bdrv_primary_bs(bs);
> +    if (!primary_bs) {
> +        return -ENOTSUP;
> +    }
> +
> +    return bdrv_get_allocated_file_size(primary_bs);
> +}

This can become static, too (never used as a callback), and possibly
even be inlined.

> +/**
> + * Implementation of BlockDriver.bdrv_get_allocated_file_size() for
> + * protocol block drivers that just do not support it.
> + */
> +int64_t bdrv_notsup_allocated_file_size(BlockDriverState *bs)
> +{
> +    return -ENOTSUP;
> +}

Also never used as a callback. I think inlining it would almost
certainly make more sense.

Kevin