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 <vsementsov@virtuozzo.com>
>>>> Signed-off-by: Max Reitz <mreitz@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 <vsementsov@virtuozzo.com>