Re: [Qemu-devel] [PATCH v6 22/42] block: Fix bdrv_get_allocated_file_size's fallback

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
>>> Signed-off-by: Max Reitz <address@hidden>
>>> ---
>>>    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] ?

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]

with or without any of these suggestions:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

-- 
Best regards,
Vladimir