On 22.06.20 16:46, Alberto Garcia wrote:
> On Mon 22 Jun 2020 11:35:59 AM CEST, Max Reitz wrote:
>>>> +    if (qcow2_opts->data_file_raw &&
>>>> +        qcow2_opts->preallocation == PREALLOC_MODE_OFF)
>>>> +    {
>>>> +        /*
>>>> +         * data-file-raw means that "the external data file can be
>>>> +         * read as a consistent standalone raw image without looking
>>>> +         * at the qcow2 metadata."  It does not say that the metadata
>>>> +         * must be ignored, though (and the qcow2 driver in fact does
>>>> +         * not ignore it), so the L1/L2 tables must be present and
>>>> +         * give a 1:1 mapping, so you get the same result regardless
>>>> +         * of whether you look at the metadata or whether you ignore
>>>> +         * it.
>>>> +         */
>>>> +        qcow2_opts->preallocation = PREALLOC_MODE_METADATA;
>>>
>>> I'm not convinced by this,
>>
>> Why not?
>>
>> This is how I read the spec.  Furthermore, I see two problems that we
>> have right now that are fixed by this patch (namely (1) using a device
>> file as the external data file, which may have non-zero data at
>> creation; and (2) assigning a backing file at runtime must not show
>> the data).
> 
> What happens if you first create the image (which would be preallocated
> with this patch), then you resize it and finally you assign the backing
> file? Would the resized part be preallocated?

Good point, when resizing an image with data-file-raw we also need to
preallocate the L2 tables.

>>> but your comment made me think of another possible alternative: in
>>> qcow2_get_cluster_offset(), if the cluster is unallocated and we are
>>> using a raw data file then we return _ZERO_PLAIN:
>>>
>>> --- a/block/qcow2-cluster.c
>>> +++ b/block/qcow2-cluster.c
>>> @@ -654,6 +654,10 @@ out:
>>>      assert(bytes_available - offset_in_cluster <= UINT_MAX);
>>>      *bytes = bytes_available - offset_in_cluster;
>>>  
>>> +    if (type == QCOW2_CLUSTER_UNALLOCATED && data_file_is_raw(bs)) {
>>> +        type = QCOW2_CLUSTER_ZERO_PLAIN;
>>> +    }
>>> +
>>>      return type;
>>>
>>> You could even add a '&& bs->backing' to the condition and emit a
>>> warning to make it more explicit.
>>
>> No, this is wrong.  This still wouldn’t fix the problem of having a
>> device file as the external data file, when it already has non-zero
>> data during creation.  (Reading the qcow2 file would return zeroes,
>> but reading the device would not.)
> 
> But you wouldn't fix that preallocating the metadata either, you would
> need to fill the device with zeroes.

What it fixes is that reading the qcow2 image and the raw device returns
the same data.

Initially, I also thought that we should initialize raw data files to be
zero during creation, but Eric changed my mind:

https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00223.html

>> I interpret the spec in that the metadata can be ignored, but it does
>> not need to be ignored.  So the L1/L2 tables must be 1:1 mapping of
>> QCOW2_CLUSTER_NORMAL entries.
>>
>> We could also choose to interpret it as “With data-file-raw, the L1/L2
>> tables must be ignored”.  In that case, our qcow2 driver would need to
>> be modified to indeed fully ignore the L1/L2 tables with
>> data-file-raw.  (I certainly don’t interpret the spec this way, but I
>> suppose we could call it a bug fix and amend it.)
> 
> The way I interpret it is that regardless of whether you read the data
> through the qcow2 file or directly from the data file you should get the
> same results, but how that should be reflected in the L1/L2 metadata is
> not specified.

That’s an absolute given, but the question is what does “reading through
the qcow2 file” mean.  Respecting the metadata?  Ignoring it?  Something
in between?

As I noted in my reply to myself, data-file-raw is an autoclear flag.
That means, an old version of qemu that doesn’t recognize the flag must
read the same data as a new version.  It follows that the the L2 tables
must be a 1:1 mapping.  (Or the flag can’t be an autoclear flag.)

Max