Re: [Qemu-devel] [RFC PATCH v2 5/6] qcow2: implement bdrv_preallocate

[Peter Lieven]

Am 27.11.2013 11:07, schrieb Fam Zheng:
> On 2013年11月27日 18:03, Peter Lieven wrote:
>> Am 27.11.2013 07:40, schrieb Fam Zheng:
>>> On 2013年11月27日 14:01, Hu Tao wrote:
>>>> On Wed, Nov 27, 2013 at 11:01:23AM +0800, Fam Zheng wrote:
>>>>> On 2013年11月27日 10:15, Hu Tao wrote:
>>>>>> Signed-off-by: Hu Tao <address@hidden>
>>>>>> ---
>>>>>>    block/qcow2.c | 7 +++++++
>>>>>>    1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>> index b054a01..a23fade 100644
>>>>>> --- a/block/qcow2.c
>>>>>> +++ b/block/qcow2.c
>>>>>> @@ -2180,6 +2180,12 @@ static int qcow2_amend_options(BlockDriverState 
>>>>>> *bs,
>>>>>>        return 0;
>>>>>>    }
>>>>>>
>>>>>> +static int qcow2_preallocate(BlockDriverState *bs, int64_t offset,
>>>>>> +                             int64_t length)
>>>>>> +{
>>>>>> +    return bdrv_preallocate(bs->file, offset, length);
>>>>>> +}
>>>>>> +
>>>>>
>>>>> What's the semantics of .bdrv_preallocate? I think you should map
>>>>> [offset, offset + length) to clusters in image file, and then
>>>>> forward to bs->file, rather than this direct wrapper.
>>>>>
>>>>> E.g. bdrv_preallocate(qcow2_bs, 0, cluster_size) should call
>>>>> bdrv_preallocate(qcow2_bs->file, offset_off_first_cluster,
>>>>> cluster_size).
>>>>
>>>> You mean data clusters here, right? Is there a single function to get
>>>> the offset of the first data cluster?
>>>>
>>>
>>> There is a function, qcow2_get_cluster_offset.
>> This should return no valid offset as long as the cluster is not allocated.
>>
>> I think you actually have to "write" all clusters of a qcow2 one by one.
>> Eventually this write could be an fallocate call instead of a zero write.
>>
>
> Yes, I was wrong about qcow2_get_cluster_offset. The logic here is more like 
> cluster allocation in qcow2_alloc_cluster_offset. Maybe we can reuse that.
What I don't like about the preallocation is that we would loose the 
information that a cluster contains no valid data and would read it e.g. during
conversion.

I think what we want is a preallocated image with all clusters sequentally 
mapped into the qcow2 file. Preallocate all the cluster extends, but still
have the information in the table that the cluster in fact has no valid data. 
So we would need a valid cluster offset while still haveing the
flag that the cluster is unallocated. I think this would require thoughtfully 
checking all the cluster functions if they can easily cope with this.

The quetion is Hu, what do you want to achieve? Do you want that the space on 
the filesystem is preallocated so you can't overcommit or
do you also want a sequential mapping of all the clusters into the file?

Peter