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] ?
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 <vsement...@virtuozzo.com>
--
Best regards,
Vladimir
