On 12.08.19 19:14, Vladimir Sementsov-Ogievskiy wrote:
> 12.08.2019 16:09, Max Reitz wrote:
>> On 10.08.19 18:41, Vladimir Sementsov-Ogievskiy wrote:
>>> 09.08.2019 19:13, Max Reitz wrote:
>>>> If the driver does not implement bdrv_get_allocated_file_size(), we
>>>> should fall back to cumulating the allocated size of all non-COW
>>>> children instead of just bs->file.
>>>>
>>>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
>>>> Signed-off-by: Max Reitz <mre...@redhat.com>
>>>> ---
>>>> block.c | 22 ++++++++++++++++++++--
>>>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index 1070aa1ba9..6e1ddab056 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -4650,9 +4650,27 @@ int64_t
>>>> bdrv_get_allocated_file_size(BlockDriverState *bs)
>>>> if (drv->bdrv_get_allocated_file_size) {
>>>> return drv->bdrv_get_allocated_file_size(bs);
>>>> }
>>>> - if (bs->file) {
>>>> - return bdrv_get_allocated_file_size(bs->file->bs);
>>>> +
>>>> + if (!QLIST_EMPTY(&bs->children)) {
>>>> + BdrvChild *child;
>>>> + int64_t child_size, total_size = 0;
>>>> +
>>>> + QLIST_FOREACH(child, &bs->children, next) {
>>>> + if (child == bdrv_filtered_cow_child(bs)) {
>>>> + /* Ignore COW backing files */
>>>> + continue;
>>>> + }
>>>> +
>>>> + child_size = bdrv_get_allocated_file_size(child->bs);
>>>> + if (child_size < 0) {
>>>> + return child_size;
>>>> + }
>>>> + total_size += child_size;
>>>> + }
>>>> +
>>>> + return total_size;
>>>> }
>>>> +
>>>> return -ENOTSUP;
>>>> }
>>>>
>>>>
>>>
>>> Hmm..
>>>
>>> 1. No children -> -ENOTSUP
>>> 2. Only cow child -> 0
>>> 3. Some non-cow children -> SUM
>>>
>>> It's all arguable (the strictest way is -ENOTSUP in either case),
>>> but if we want to fallback to SUM of non-cow children, 1. and 2. should
>>> return
>>> the same.
>>
>> I don’t think 2 is possible at all. If you have a COW child, you need
>> some other child to COW to.
>>
>> And in the weird (and probably impossible) case where a node really only
>> has a COW child, I’d say it’s correct that it has a disk size of 0 –
>> because it hasn’t COWed anything yet. (Just like a new qcow2 image with
>> a backing file only has its metadata as its disk size.)
>>
>
> Agreed. Then, why not return 0 on [1] ?(1) Because that’s the current behavior. :-) (2) Nodes that have no children are protocol nodes. Protocol nodes (apart from null) still have to store their data somewhere. Therefore, they must implement .bdrv_get_allocated_file_size() to report that. If they don’t, that doesn’t mean they don’t store any data, but only that we don’t know how much data they store. > Also, another idea: shouldn't we return 0 for filters, i.e. skip > filtered_rw_child too? > [as filtered-child is more like backing child than file one, it's "less > owned" by its parent] Why would we do that? If I have a block device with a throttle node attached to it and request how much space it uses, of course I will want to know how much space the whole tree below it uses. (Otherwise, bdrv_get_allocated_file_size() should only report anything for protocol nodes, and 0 for everything else.) Max > with or without any of these suggestions: > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
signature.asc
Description: OpenPGP digital signature
