Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw

[Alberto Garcia]

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?

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

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

Berto