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

[Max Reitz]

On 11.09.19 08:55, Kevin Wolf wrote:
> Am 11.09.2019 um 08:20 hat Max Reitz geschrieben:
>> On 10.09.19 16:52, Kevin Wolf wrote:
>>> Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
>>>> 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>
>>>
>>> This smells like an overgeneralisation, but if we want to count all vmdk
>>> extents, the qcow2 external data file, etc. it's an improvement anyway.
>>> A driver that has a child that should not be counted must just remember
>>> to implement the callback.
>>>
>>> Let me think of an example... How about quorum, for a change? :-)
>>> Or the second blkverify child.
>>>
>>> Or eventually the block job filter nodes.
>>
>> I actually think it makes sense for all of these nodes to report the sum
>> of all of their children’s allocated sizes.
> 
> Hm... Yes, in a way. But not much more than it would make sense to
> report the sum of the sizes of all images in the whole backing chain
> (this is a useful thing to ask for, just maybe not the right thing to
> return for a low-level interface). But I can accept that it's maybe a
> bit more expected for quorum and blkverify than for COW images.
> 
> If you include the block job filter nodes, I have to disagree, though.
> If mirror_top_bs (or any other job filter) sits in the middle of the
> source chain, then I certainly don't want to see the target size added
> to it.

Hm, I don’t care much either way.  I think it makes complete sense to
add the target size there, but OTOH it’s only temporary while the job
runs, so it may be a bit confusing if it suddenly goes up and then down
again.

But I think this is the special case, so this is what should be handled
in a driver callback.

>> If a quorum node has three children with allocated sizes of 3 MB, 1 MB,
>> and 2 MB, respectively (totally possible if some have explicit zeroes
>> and others don’t; it may also depend on the protocol, the filesystem,
>> etc.), then I think it makes most sense to report indeed 6 MB for the
>> quorum subtree as a whole.  What would you report?  3 MB?
> 
> Do it's the quorum way: Just vote!

Add an option for it?  Average, maximum, median, majority, sum? :-)

> No, you're right, of course. -ENOTSUP is probably the only other thing
> you could do then.
> 
>>> Ehm... Maybe I should just take back what I said first. It almost feels
>>> like it would be better if qcow2 and vmdk explicitly used a handler that
>>> counts all children (could still be a generic one in block.c) rather
>>> than having to remember to disable the functionality everywhere where we
>>> don't want to have it.
>>
>> I don’t, because everywhere we don’t want this functionality, we still
>> need to choose a child.  This has to be done by the driver anyway.
> 
> Well, by default the primary child, which should cover like 90% of the
> drivers?

Hm, yes.

But I still think that the drivers that do not want to count every
single non-COW child are the exception.

Max

signature.asc
Description: OpenPGP digital signature