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

[Max Reitz]

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.

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?

> 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.

Max

> And please adjust the comment for bdrv_get_allocated_file_size(), it
> only talks about a single file as if trees didn't exist. Actually, it
> doesn't even seem so easy to define. Maybe primary node + storage nodes?
> Then vmdk needs to expose its extents as storage nodes (plural!), but
> in the long run that might be needed anyway.

signature.asc
Description: OpenPGP digital signature