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

[Max Reitz]

On 22.06.20 17:15, Nir Soffer wrote:
> On Mon, Jun 22, 2020 at 6:07 PM Max Reitz <mreitz@redhat.com> wrote:
>>
>> 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.)
> 
> Being able to read sounds like a nice to have feature, but what about writing?
> 
> I hope that the image is not writable by older versions that do not understand
> data_file. Otherwise older qemu versions can corrupt the image silently.

It’s an autoclear flag.  That means such versions of qemu will
automatically clear the flag.

(To elaborate, there are three kinds of flags for qcow2 images:
Incompatible flags, compatible flags, and autoclear flags.

When qemu encounters an image with an unknown incompatible flag, it
refuses to open the image.

When it encounters an unknown compatible flag, it just ignores that flag
(but keeps it set).

When it encounters an unknown autoclear flag, it will clear that flag
and then continue as if it hadn’t been present.

So autoclear flags are useful for features that are optional, but that
may be broken when the image is written to by versions of qemu that
don’t understand them.)

Max

signature.asc
Description: OpenPGP digital signature